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

src/jrd/Constant.epp

doc/sql.extensions/README.packages.txt

src/burp/OdsDetection.epp

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.

src/dsql/DdlNodes.epp

src/jrd/relations.h

src/jrd/SysFunction.cpp

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.

src/dsql/DdlNodes.epp

src/dsql/ExprNodes.h

src/burp/OdsDetection.epp

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?

Changed to false. The logic moved to the isChildrenConstant() method. Child methods will call it when needed.

@Noremos I repeat my question: what is the difference between constant and deterministic methods?

When I started implementing constants, I asked @dyemanov if I could use the deterministic() method to check the constant status of an expression. Dmitry responded that this would be incorrect. So, please wait for Dmitry's reply.

src/dsql/ExprNodes.cpp

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?

Yes, it is reserved for global constants, but to be honest, I haven't looked into the security code so I'm not sure if it's really necessary for global constants

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.

I added this because it is necessary to Routine class

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?

I'm relying on the rel_funs case, and it simply calls Function::lookup(tdbb, id, 0); So I assume the case where the constant isn't found is impossible. I hope an assert will be enough. Or should I add an exception?

I should clarify. I think the main purpose of the lookup method isn't to check for existence, but to populate the cache, since it has an auto-create flag. But I could be wrong.

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?

…ence

Noremos · 2026-03-01T13:17:17Z

No code for backup and restore the new table?

It seems like the changes were lost during some merge or migration. I added the changes again. Thanks for noticing

Noremos · 2026-03-01T13:20:48Z

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

I'll try. I hope I can make as few changes as possible.

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

src/jrd/Constant.epp Outdated Show resolved Hide resolved

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

Artyom Abakumov added 5 commits March 1, 2026 14:04

Fix type in test

46e0e5d

Omit unneccecery getConstant method and add assert for constant exist…

51d9ab6

…ence

Mark RecordKeyNode as non constant

2ccea0f

Add missing code for backup-restore

8d85f05

Add missing source for windows build

2f11ba4

sim1984 mentioned this pull request Mar 1, 2026

Allow to define const(s) to database #8921

Open

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!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

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!

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!

asfernandes commented Feb 28, 2026

Uh oh!

Noremos commented Mar 1, 2026

Uh oh!

Noremos commented Mar 1, 2026

Uh oh!

Noremos commented Feb 25, 2026 •

edited

Loading

Noremos Feb 27, 2026 •

edited

Loading