Skip to content

Fix issues with longish concatenated context aliases #8494

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
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix issues with longish concatenated context aliases #8494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 20 additions & 6 deletions src/jrd/RecordSourceNodes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,20 @@ static ValueExprNode* resolveUsingField(DsqlCompilerScratch* dsqlScratch, const

namespace
{
void appendContextAlias(DsqlCompilerScratch* dsqlScratch, const string& alias)
{
const auto len = alias.length();
if (len <= MAX_UCHAR)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be more advantageous to use MAX_SQL_IDENTIFIER_LEN rather than MAX_UCHAR, considering the possibility of MetaName length expanding in the future?

Copy link
Member Author

@dyemanov dyemanov Apr 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Alias is a MetaName inside the query, but it's a string for the optimizer (because it's concatenated from the whole aliases stack for nested contexts like derived tables).
  2. String length inside BLR is limited by 1 byte, hence the check for MAX_UCHAR. It would be a PITA to extend it (without introducing new BLR version).

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks for the clarification.

dsqlScratch->appendMetaString(alias.c_str());
else
{
string truncatedAlias(alias);
truncatedAlias.resize(MAX_UCHAR - 3);
truncatedAlias += "...";
Copy link
Member

Choose a reason for hiding this comment

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

How this change of a name overflow to ellipsis will resolve problems?

Copy link
Member Author

Choose a reason for hiding this comment

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

It avoids an assertion (debug build) or weird alias truncation (release build) inside BlrWriter::appendString when called from genBlr(). Of course, we could just truncate the string at the 255 bytes boundary, but IMHO ellipsis look better as clearly shows the fact of truncation. Now BLR is generated (and later parsed) properly and ellipsis is shown inside the very long plan's alias, AFAIS without any additional side effects.

dsqlScratch->appendMetaString(truncatedAlias.c_str());
}
}

// Search through the list of ANDed booleans to find comparisons
// referring streams of parent select expressions.
// Extract those booleans and return them to the caller.
Expand Down Expand Up @@ -687,7 +701,7 @@ void LocalTableSourceNode::genBlr(DsqlCompilerScratch* dsqlScratch)
{
dsqlScratch->appendUChar(blr_local_table_id);
dsqlScratch->appendUShort(tableNumber);
dsqlScratch->appendMetaString(alias.c_str()); // dsqlContext->ctx_alias?
appendContextAlias(dsqlScratch, alias); // dsqlContext->ctx_alias?

GEN_stuff_context(dsqlScratch, dsqlContext);
}
Expand Down Expand Up @@ -885,7 +899,7 @@ void RelationSourceNode::genBlr(DsqlCompilerScratch* dsqlScratch)
}

if (dsqlContext->ctx_alias.hasData())
dsqlScratch->appendMetaString(dsqlContext->ctx_alias.c_str());
appendContextAlias(dsqlScratch, dsqlContext->ctx_alias);

GEN_stuff_context(dsqlScratch, dsqlContext);
}
Expand Down Expand Up @@ -1507,7 +1521,7 @@ void ProcedureSourceNode::genBlr(DsqlCompilerScratch* dsqlScratch)
if (dsqlContext->ctx_alias.hasData())
{
dsqlScratch->appendUChar(blr_invsel_procedure_alias);
dsqlScratch->appendMetaString(dsqlContext->ctx_alias.c_str());
appendContextAlias(dsqlScratch, dsqlContext->ctx_alias);
}

dsqlScratch->appendUChar(blr_end);
Expand All @@ -1519,7 +1533,7 @@ void ProcedureSourceNode::genBlr(DsqlCompilerScratch* dsqlScratch)
{
dsqlScratch->appendUChar(blr_subproc);
dsqlScratch->appendMetaString(dsqlProcedure->prc_name.identifier.c_str());
dsqlScratch->appendMetaString(dsqlContext->ctx_alias.c_str());
appendContextAlias(dsqlScratch, dsqlContext->ctx_alias);
}
else
{
Expand All @@ -1546,7 +1560,7 @@ void ProcedureSourceNode::genBlr(DsqlCompilerScratch* dsqlScratch)
}

if (dsqlContext->ctx_alias.hasData())
dsqlScratch->appendMetaString(dsqlContext->ctx_alias.c_str());
appendContextAlias(dsqlScratch, dsqlContext->ctx_alias);
}

GEN_stuff_context(dsqlScratch, dsqlContext);
Expand Down Expand Up @@ -4079,7 +4093,7 @@ void TableValueFunctionSourceNode::genBlr(DsqlCompilerScratch* dsqlScratch)

GEN_stuff_context(dsqlScratch, dsqlContext);

dsqlScratch->appendMetaString(dsqlContext->ctx_alias.c_str());
appendContextAlias(dsqlScratch, dsqlContext->ctx_alias);

dsqlScratch->appendUShort(dsqlContext->ctx_proc_inputs->items.getCount());
for (auto& arg : dsqlContext->ctx_proc_inputs->items)
Expand Down
2 changes: 1 addition & 1 deletion src/jrd/recsrc/BitmapTableScan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,6 @@ void BitmapTableScan::internalGetPlan(thread_db* tdbb, PlanEntry& planEntry, uns
planEntry.objectType = m_relation->getObjectType();
planEntry.objectName = m_relation->rel_name;

if (m_alias.hasData() && m_relation->rel_name != m_alias)
if (m_alias.hasData() && m_alias != m_relation->rel_name.c_str())
planEntry.alias = m_alias;
}
2 changes: 1 addition & 1 deletion src/jrd/recsrc/ExternalTableScan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,6 @@ void ExternalTableScan::internalGetPlan(thread_db* tdbb, PlanEntry& planEntry, u
planEntry.objectType = m_relation->getObjectType();
planEntry.objectName = m_relation->rel_name;

if (m_alias.hasData() && m_relation->rel_name != m_alias)
if (m_alias.hasData() && m_alias != m_relation->rel_name.c_str())
planEntry.alias = m_alias;
}
2 changes: 1 addition & 1 deletion src/jrd/recsrc/FullTableScan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,6 @@ void FullTableScan::internalGetPlan(thread_db* tdbb, PlanEntry& planEntry, unsig
planEntry.objectType = m_relation->getObjectType();
planEntry.objectName = m_relation->rel_name;

if (m_alias.hasData() && m_relation->rel_name != m_alias)
if (m_alias.hasData() && m_alias != m_relation->rel_name.c_str())
planEntry.alias = m_alias;
}
2 changes: 1 addition & 1 deletion src/jrd/recsrc/IndexTableScan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ void IndexTableScan::internalGetPlan(thread_db* tdbb, PlanEntry& planEntry, unsi
planEntry.objectType = m_relation->getObjectType();
planEntry.objectName = m_relation->rel_name;

if (m_alias.hasData() && m_relation->rel_name != m_alias)
if (m_alias.hasData() && m_alias != m_relation->rel_name.c_str())
planEntry.alias = m_alias;

if (m_inversion)
Expand Down
2 changes: 1 addition & 1 deletion src/jrd/recsrc/RecordSource.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ namespace Jrd
std::optional<ObjectType> objectType;
MetaName packageName;
MetaName objectName;
MetaName alias;
Firebird::string alias{getPool()};
const AccessPath* accessPath = nullptr;
ULONG recordLength = 0;
ULONG keyLength = 0;
Expand Down
2 changes: 1 addition & 1 deletion src/jrd/recsrc/VirtualTableScan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,6 @@ void VirtualTableScan::internalGetPlan(thread_db* tdbb, PlanEntry& planEntry, un
planEntry.objectType = m_relation->getObjectType();
planEntry.objectName = m_relation->rel_name;

if (m_alias.hasData() && m_relation->rel_name != m_alias)
if (m_alias.hasData() && m_alias != m_relation->rel_name.c_str())
planEntry.alias = m_alias;
}
Loading