Skip to content

Fix collate and null processing in package constants #9037

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Noremos wants to merge 2 commits into
base: master
Choose a base branch
from
+37 −13
Open

Fix collate and null processing in package constants #9037

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b44d11d
Fix null values processing in constants
May 25, 2026
48de76b
Allow collate in constant declaration
May 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/dsql/PackageNodes.epp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,7 @@ void PackageReferenceNode::make(DsqlCompilerScratch* dsqlScratch, dsc* desc)
jrd_tra* transaction = dsqlScratch->getTransaction();
thread_db* tdbb = JRD_get_thread_data();
*desc = ConstantValue::getDesc(tdbb, transaction, m_fullName);
desc->setNullable(true);
Copy link
Copy Markdown
Member

@asfernandes asfernandes May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is constant references always marked as nullable?

Copy link
Copy Markdown
Contributor Author

@Noremos Noremos May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To return NULL from the ReferenceNode, the nullable flag is requered. It would be nice to explicitly check the not null state of the field, but as I can see, it does not stored in RDB$FIELD. So I decied to put the nullable flag withou extra checks.

The not null clasue can only be used when the type is a domain:

create domain my_domain int not null;

create or alter package my_package as
begin
    constant my_const my_domain = null;
end;

But in this case an error will appear instanly:

Statement failed, SQLSTATE = 2F000
validation error for CAST, value "*** null ***"
-Error while parsing BLR value of the constant "PUBLIC"."MY_PACKAGE"."MY_CONST"

So, putting nullable flag in any case is safe

Copy link
Copy Markdown
Member

@asfernandes asfernandes May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every other kind of expressions, include composed ones, do return nullable flag correctly. I see no reason for constants to flag nullable incorrectly.

Copy link
Copy Markdown
Member

@dyemanov dyemanov May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to explicitly check the not null state of the field, but as I can see, it does not stored in RDB$FIELD

RDB$FIELDS.RDB$NULL_FLAG ?

}

bool PackageReferenceNode::constantExists(thread_db* tdbb, Jrd::jrd_tra* transaction,
Expand Down Expand Up @@ -503,6 +504,7 @@ bool PackageReferenceNode::constantExists(thread_db* tdbb, Jrd::jrd_tra* transac
void PackageReferenceNode::getDesc(thread_db* tdbb, CompilerScratch*, dsc* desc)
{
*desc = ConstantValue::getDesc(tdbb, tdbb->getTransaction(), m_fullName);
desc->setNullable(true);
}

ValueExprNode* PackageReferenceNode::copy(thread_db* tdbb, NodeCopier& copier) const
Expand Down Expand Up @@ -550,15 +552,19 @@ ValueExprNode* PackageReferenceNode::pass2(thread_db* tdbb, CompilerScratch* csb

dsc* PackageReferenceNode::execute(thread_db* tdbb, Request* request) const
{
impure_value* outputImpure = request->getImpure<impure_value>(m_impureOffset);
switch (m_itemType)
{
case blr_pkg_reference_to_constant:
{
Package* package = m_package(request->getResources());
package->checkReload(tdbb);

EVL_make_value(tdbb, package->findConstant(m_fullName)->makeValue(tdbb, request), outputImpure);
dsc* constantDsc = package->findConstant(m_fullName)->makeValue(tdbb, request);
if (constantDsc == nullptr || constantDsc->isNull())
return nullptr;

impure_value* outputImpure = request->getImpure<impure_value>(m_impureOffset);
EVL_make_value(tdbb, constantDsc, outputImpure);
return &outputImpure->vlu_desc;
}
default:
Expand Down
7 changes: 4 additions & 3 deletions src/dsql/parse.y
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3338,10 +3338,11 @@ replace_package_body_clause

%type <createPackageConstantNode> package_const_item
package_const_item
: symbol_package_const_name data_type_descriptor '=' value
: symbol_package_const_name data_type_descriptor collate_clause '=' value
{
$$ = newNode<CreatePackageConstantNode>(*$1, $2, $4);
$$->source = makeParseStr(YYPOSNARG(3), YYPOSNARG(4));
setCollate($2, $3);
$$ = newNode<CreatePackageConstantNode>(*$1, $2, $5);
$$->source = makeParseStr(YYPOSNARG(4), YYPOSNARG(5));
}
;

Expand Down
30 changes: 22 additions & 8 deletions src/jrd/Package.epp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,12 +156,12 @@ dsc* ConstantValue::makeValue(thread_db* tdbb, Request* request)
{
{
ReadLockGuard guard(m_makeValueLock, FB_FUNCTION);
if (m_value.vlu_desc.dsc_address != nullptr)
if (m_value.vlu_desc.dsc_dtype != 0)
return &m_value.vlu_desc;
}

WriteLockGuard guard(m_makeValueLock, FB_FUNCTION);
if (m_value.vlu_desc.dsc_address != nullptr) // Extra check in case of two callers waiting
if (m_value.vlu_desc.dsc_dtype != 0) // Extra check in case of two callers waiting
return &m_value.vlu_desc;

Attachment* attachment = tdbb->getAttachment();
Expand Down Expand Up @@ -196,14 +196,11 @@ dsc* ConstantValue::makeValue(thread_db* tdbb, Request* request)

TRA_attach_request(tdbb->getTransaction(), request);

const dsc* temp = EVL_expr(tdbb, request, static_cast<ValueExprNode*>(csb->csb_node));
EVL_make_value(tdbb, temp, &m_value, &getPool());
//! Execute it here, while AutoSetRestore2 is active
executeCsbNode(tdbb, csb);
}
else
{
const dsc* temp = EVL_expr(tdbb, request, static_cast<ValueExprNode*>(csb->csb_node));
EVL_make_value(tdbb, temp, &m_value, &getPool());
}
executeCsbNode(tdbb, csb);
}
catch (const Exception& ex)
{
Expand All @@ -226,6 +223,23 @@ dsc* ConstantValue::makeValue(thread_db* tdbb, Request* request)
return &m_value.vlu_desc;
}

void ConstantValue::executeCsbNode(thread_db* tdbb, CompilerScratch* csb)
{
ValueExprNode* exprNode = static_cast<ValueExprNode*>(csb->csb_node);
fb_assert(exprNode);

const dsc* temp = EVL_expr(tdbb, tdbb->getRequest(), exprNode);
if (temp != nullptr)
{
EVL_make_value(tdbb, temp, &m_value, &getPool());
}
else
{
exprNode->getDesc(tdbb, csb, &m_value.vlu_desc);
m_value.vlu_desc.setNull();
}
}

ConstantValue& ConstantsCache::add(const QualifiedName& constName, const bool isPrivate)
{
fb_assert(!nameMap.exist(constName));
Expand Down
3 changes: 3 additions & 0 deletions src/jrd/Package.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@

namespace Jrd
{
class CompilerScratch;
class DsqlCompilerScratch;
class dsql_fld;

Expand Down Expand Up @@ -84,6 +85,8 @@ class ConstantValue final : public Firebird::PermanentStorage
dsc* makeValue(thread_db* tdbb, Request* request);

private:
void executeCsbNode(thread_db* tdbb, CompilerScratch* csb);

// Lock in case of makeing value during the execute state
Firebird::RWLock m_makeValueLock{};

Expand Down
Loading