Package constants by Noremos · Pull Request #8916 · FirebirdSQL/firebird

Noremos · 2026-02-25T08:05:08Z

This PR adds a new database object - Package Constant (#1036). See README.packages.txt for more information.
Usage examples:

set autoterm;
CREATE PACKAGE MY_PACKAGE
AS
BEGIN
    CONSTANT PUBLIC_CONST INTEGER = 10;
    FUNCTION MY_SECRET_PRINT() RETURNS INT;
END;

CREATE PACKAGE BODY MY_PACKAGE
AS
BEGIN
    CONSTANT SECRET_1 INTEGER = 15;
    CONSTANT SECRET_2 INTEGER = 30;

    FUNCTION MY_SECRET_PRINT() RETURNS INT
    AS
    BEGIN
        RETURN SECRET_1 * SECRET_2;
    END
END;
commit;

select MY_PACKAGE.PUBLIC_CONST from rdb$database ;
select MY_PACKAGE.MY_SECRET_PRINT() from rdb$database;

The implementation has three key points:

The package constants code is flexible enough to implement package variables, global constants, and additional package elements.
All nodes now have an 'is constant' flag, similar to the 'is deterministic' flag. This is necessary to determine whether an expression can initialize a constant.
In the CreateAlterPackageNode node, a lot of code had to be copied and pasted, and adding constants led to complete chaos. Therefore, I decided to slightly refactor this node. All functionality remains the same; only code duplication was removed.

…rong impure cache usage

…stant routine

…_constants

…onstants

sim1984 · 2026-02-25T13:03:32Z

Are comments supported for constants? I didn't see this in the documentation.
Let me clarify what I mean:

COMMENT ON CONSTANT [<schema>.]<package>.<const_name> IS 'Text'

Comments are simply supported for packaged procedures and functions. It would make sense to do the same for constants (and any package objects, for that matter).

COMMENT ON PROCEDURE [<schema>.]<package>.<proc_name> IS 'Text';
COMMENT ON FUNCTION [<schema>.]<package>.<func_name> IS 'Text';

Noremos · 2026-02-25T13:16:08Z

Are comments supported for constants?

No, I didn't plan to implement comments, but I think it's possible. I will take a look

Noremos · 2026-02-25T18:22:13Z

Are comments supported for constants? I didn't see this in the documentation. Let me clarify what I mean:
COMMENT ON CONSTANT [<schema>.]<package>.<const_name> IS 'Text'

Ok, it wasn't that hard, I implemented the feature as suggested.

Examples:

SQL> comment on constant PUBLIC.MY_PACKAGE.PUBLIC_CONST is 'my description';
SQL> comment on constant MY_PACKAGE.SECRET_1 is 'The Magic Value!';

src/dsql/PackageNodes.h

AlexPeshkoff

Generally OK, just minor correction please

AlexPeshkoff · 2026-02-26T17:02:02Z

src/jrd/Constant.epp

+			if (ssDefiner.asBool())
+				invoker = dbb->getUserId(getPermanent()->owner);
+
+			makeValue(tdbb, attachment, CONST.RDB$CONSTANT_BLR);


I'm not 100% sure - is it OK to invoke makeValue() when MINISCAN is set? Certainly, constants should not have as complex dependencies as procedures/functions, but anyway - may be better leave it for reload in such a case?

Changed to:

if ((flags & CacheFlag::MINISCAN) == 0) makeValue(tdbb, attachment, CONST.RDB$CONSTANT_BLR); else this->flReload = true; // Call makeValue in reload

asfernandes · 2026-02-27T00:20:28Z

doc/sql.extensions/README.packages.txt

+      A package constant is a value initialized by a constant expression.
+      A constant expression is defined by a simple rule: its value does not change after recompilation.
+      Constants declared in the package specification are publicly visible and can be referenced using
+      the <package>.<constant_name> notation.


Add syntax about optional schema.

asfernandes · 2026-02-27T00:21:19Z

doc/sql.extensions/README.packages.txt

+      Constants declared in the package specification are publicly visible and can be referenced using
+      the <package>.<constant_name> notation.
+      Constants declared in the package body are private and cannot be accessed from outside the package.
+      However, they can be referenced directly by <constant_name> within <procedure_impl> and <function_impl>.


This appears that package header constant cannot be referenced in its own body without the qualified package name.

I corrected the desription

asfernandes · 2026-02-27T00:21:34Z

src/burp/OdsDetection.epp

 		{"RDB$PACKAGES",		0,	DB_VERSION_DDL12},	// FB3
 		{"RDB$PUBLICATIONS",	0,	DB_VERSION_DDL13},	// FB4
 		{"RDB$SCHEMAS",			0,	DB_VERSION_DDL14},	// FB6
+		{"RDB$CONSTANTS",		0,	DB_VERSION_DDL14},// RDB6


It's not needed at all, as DB_VERSION_DDL14 is already detected.

asfernandes · 2026-02-27T00:51:02Z

src/dsql/DdlNodes.epp

+		{
+			QualifiedName constantName(subName, name.schema, name.object); // name is a package
+			if (constantName.schema.isEmpty())
+				constantName.schema = PUBLIC_SCHEMA;


This was not necessary in any other command in DdlNodes.
Looks incorrect here.

Actuality, as I can see, something similar (but more more complicated) is happening for procedures and functions in resolveRoutineOrRelation(...). Without the check, scheme will remain NULL.

asfernandes · 2026-02-27T00:52:25Z

src/dsql/DdlNodes.epp

 			break;

+		case obj_package_constant:
+			SCL_check_package(tdbb, QualifiedName(name.package, name.schema), SCL_alter);


Why it's not name.getSchemaAndPackage() like for procedures and functions?

Have not known about this function. Thanks for the tip!

asfernandes · 2026-02-27T01:40:29Z

src/jrd/relations.h

 END_RELATION
+
+// Relation 59 (RDB$CONSTANTS)
+RELATION(nam_constants, rel_constants, ODS_13_1, rel_persistent)


Suggested change

RELATION(nam_constants, rel_constants, ODS_13_1, rel_persistent)

RELATION(nam_constants, rel_constants, ODS_14_0, rel_persistent)

asfernandes · 2026-02-27T01:40:55Z

src/jrd/relations.h

+RELATION(nam_constants, rel_constants, ODS_13_1, rel_persistent)
+	FIELD(f_const_name, nam_const_name, fld_f_name, 0, ODS_14_0)
+	FIELD(f_const_id, nam_const_id, fld_const_id, 0, ODS_14_0)
+	FIELD(f_const_package, nam_pkg_name, fld_pkg_name, 1, ODS_14_0)


Why are some fields using 1?

My bad. Changed to 0.

asfernandes · 2026-02-27T01:42:25Z

src/jrd/SysFunction.cpp




+constexpr auto DEFAULT = SysFunction::DETERMINISTIC | SysFunction::CONSTANT;


asfernandes · 2026-02-27T01:43:53Z

src/jrd/SysFunction.h

 class SysFunction
 {
 public:
+	enum Flags : UCHAR


enum class, please.

SysFunction::NONE is completely misleading.

enum class is problematic for flags. You have to redefine bit operations or cast the type each time. IMHO, simple enum inside a class is a good choice in such case

And could you clarify what exactly is confusing about SysFunction::NONE?

asfernandes · 2026-02-27T01:48:27Z

src/dsql/PackageNodes.epp

 #include "../jrd/Attachment.h"
 #include "../jrd/scl_proto.h"

+#include "../common/dsc_proto.h" // DSC_make_descriptor


I left the review of this file for last, and given the amount of things I saw, didn't read it.
I think the amout of changes in this file and your initial comments about it seems not correctly.
The file was maintanable and it required no extra efforts or such things in what I'm doing now (LTTs in packages).
Your approach seems incorrect for me and is making things more complicate.

I was prepared for the changes to this file to be criticized. But if things remain as is, technical debt will accumulate, and over time, everything will become unmaintainable. If @AlexPeshkoff and @dyemanov also think this changes is outside the scope of the PR, I will revert the changes to this file.

Co-authored-by: Adriano dos Santos Fernandes <529415+asfernandes@users.noreply.github.com>

dyemanov

It's the first part of the review, PackageNodes.epp will be reviewed separately.

dyemanov · 2026-02-26T12:17:29Z

src/dsql/DdlNodes.epp

 			break;

+		case obj_package_constant:
+			SCL_check_package(tdbb, QualifiedName(name.package, name.schema), SCL_alter);


Suggested change

SCL_check_package(tdbb, QualifiedName(name.package, name.schema), SCL_alter);

SCL_check_package(tdbb, name.getSchemaAndPackage(), SCL_alter);

dyemanov · 2026-02-26T13:32:47Z

src/dsql/ExprNodes.h


+	bool constant() const override
+	{
+		return true;


It should be false for RecordKeyNode.

dyemanov · 2026-02-27T04:11:20Z

src/burp/OdsDetection.epp

 		{"RDB$PACKAGES",		0,	DB_VERSION_DDL12},	// FB3
 		{"RDB$PUBLICATIONS",	0,	DB_VERSION_DDL13},	// FB4
 		{"RDB$SCHEMAS",			0,	DB_VERSION_DDL14},	// FB6
+		{"RDB$CONSTANTS",		0,	DB_VERSION_DDL14},// RDB6


It's not needed at all, as DB_VERSION_DDL14 is already detected.

dyemanov · 2026-02-27T04:47:03Z

src/dsql/Nodes.h

 	virtual bool unmappable(const MapNode* mapNode, StreamType shellStream) const;

+	// Check if expression returns constant result
+	virtual bool constant() const;


I'd define it as returning false here and override only in nodes where true should be returned. No need to pollute the codebase with the default implementation.

It actually depends on meaning of "constant result" here. For me it looks like complete duplication of deterministic(). If I'm wrong, what's the difference?

dyemanov · 2026-02-27T04:48:47Z

src/dsql/ExprNodes.cpp

+	dsql_ctx packageContext(dsqlScratch->getPool());
+	{ // Consatnts
+
+		const QualifiedName consatntName(dsqlName,


constantName

dyemanov · 2026-02-27T05:11:31Z

src/jrd/obj.h

 		case obj_schemas:
 			return "SQL$SCHEMAS";
+		case obj_package_constant:
+			return "SQL$PACKAGE_CONSTANT";


Constants belong to a package, AFAIU it cannot have it's own privileges, so there're no need in dedicated security classes. Or is it reserved for global constants, if they ever appear in the future?

dyemanov · 2026-02-28T04:48:31Z

src/jrd/lck.h

 	LCK_idx_rescan,				// Index rescan lock
 	LCK_prc_rescan,				// Procedure rescan lock
 	LCK_fun_rescan,				// Function existence lock
+	LCK_constant_rescan,		// Constant existence lock


Is it also a preparation for global constants? Because otherwise it's the package (not the constant) that should be cached/locked. Packaged constants are never altered/dropped individually, AFAIU.

dyemanov · 2026-02-28T04:56:20Z

src/jrd/Constant.h

+class DsqlCompilerScratch;
+class dsql_fld;
+
+class Constant final : public Routine


Maybe it should be refactored, so that Routine and Constant have the same base class?

dyemanov · 2026-02-28T04:58:27Z

src/jrd/vio.cpp

+			EVL_field(0, rpb->rpb_record, f_const_id, &desc2);
+			id = MOV_get_long(tdbb, &desc2, 0);
+
+			Constant::lookup(tdbb, id);


You don't use the result. Is it expected to throw if a constant is not found? Maybe some other method -- e.g. checkExistance() -- is worth adding for readability?

dyemanov · 2026-02-28T05:17:43Z

src/dsql/ExprNodes.h

 	void genBlr(DsqlCompilerScratch* dsqlScratch) override;
 	void make(DsqlCompilerScratch* dsqlScratch, dsc* desc) override;

+	bool constant() const override


I may agree with Adriano from the safety POV (new nodes are never introduced as constant by mistake), but I don't like polluting the every class with the common code.

And this means that deterministic() should also be changed -- not something I object but I'd prefer to see a clearer code.

asfernandes · 2026-02-28T17:10:18Z

No code for backup and restore the new table?

Artyom Abakumov added 22 commits February 11, 2026 13:28

Implement package constants

5aa9116

Add "is constant" flag to nodes

251615c

Fix blr parsing, generate unique names for constant FIELD_NAME, fix w…

2d35ecc

…rong impure cache usage

Improve error message for non constant expression

75db5ab

Add missing SCHEMA set to package constants, fix incorrect cached con…

e54c9e6

…stant routine

Add missing resource allocation and transfer for constants

45d62b7

Fix invalid constant and package name in store_dependencies

06411d7

Fix invalid constant drop

3dc1d3d

Fix constant evaluating in BLR Parse

d048560

Fix constants collecting and dtype_dec64 parsing

6231dbc

Fix incorrect ambiguity check

05342aa

Update support for complex dtypes

459354f

Cleanup

3c2c61e

Rollback unneccecery changes

fd208d1

Move all constant related routines to Constant class

8a982ac

Add description for package constants

c42bd55

Cleanup

4a556aa

Delete string in impure

44a7050

Merge remote-tracking branch 'upstream/master' into metacache_package…

c2caa1b

…_constants

Merge remote-tracking branch 'origin/master' into metacache_package_c…

4294bc4

…onstants

Resolve incorrect merge with LTT

3148971

One more cleanup (preparation) before PR

51fc8d7

dyemanov assigned dyemanov and unassigned dyemanov Feb 25, 2026

dyemanov self-requested a review February 25, 2026 08:09

dyemanov assigned Noremos Feb 25, 2026

dyemanov added type: new feature component: engine labels Feb 25, 2026

Add ability to add comment on package constant

bd085c9

AlexPeshkoff reviewed Feb 26, 2026

View reviewed changes

src/dsql/PackageNodes.h Outdated Show resolved Hide resolved

AlexPeshkoff requested changes Feb 26, 2026

View reviewed changes

AlexPeshkoff reviewed Feb 26, 2026

View reviewed changes

Artyom Abakumov added 3 commits February 27, 2026 01:10

Remove unneccecery namespace scope

4695050

Make Constant value in reload state when MINISCAN is set

e606d59

Remove not used variable and omit unneccecery namespace scope

cf7929e

asfernandes self-requested a review February 27, 2026 01:08

asfernandes requested changes Feb 27, 2026

View reviewed changes

Noremos and others added 5 commits February 27, 2026 10:38

Update src/isql/show.epp

12a7b2b

Co-authored-by: Adriano dos Santos Fernandes <529415+asfernandes@users.noreply.github.com>

First part of fixes/corrections related to PR issues

edca2c5

Resolve more PR issues

cfdf444

Add missing search for FIELD SCHEMA

816ad9c

Use better name for method

13b7df9

dyemanov requested changes Feb 28, 2026

View reviewed changes

	RELATION(nam_constants, rel_constants, ODS_13_1, rel_persistent)
	RELATION(nam_constants, rel_constants, ODS_14_0, rel_persistent)




		constexpr auto DEFAULT = SysFunction::DETERMINISTIC \| SysFunction::CONSTANT;

	SCL_check_package(tdbb, QualifiedName(name.package, name.schema), SCL_alter);
	SCL_check_package(tdbb, name.getSchemaAndPackage(), SCL_alter);

Uh oh!

Conversation

Noremos commented Feb 25, 2026

Uh oh!

sim1984 commented Feb 25, 2026

Uh oh!

Noremos commented Feb 25, 2026

Uh oh!

Noremos commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AlexPeshkoff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Noremos Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dyemanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Noremos commented Feb 25, 2026 •

edited

Loading

Noremos Feb 27, 2026 •

edited

Loading