Skip to content

Generate query jumble automaticly - #1827

Open
chenjinbao1989 wants to merge 5 commits into
apache:mainfrom
chenjinbao1989:generate_queryjumble
Open

Generate query jumble automaticly#1827
chenjinbao1989 wants to merge 5 commits into
apache:mainfrom
chenjinbao1989:generate_queryjumble

Conversation

@chenjinbao1989

Copy link
Copy Markdown
Contributor

Fixes #ISSUE_Number

What does this PR do?

Type of Change

  • Bug fix (non-breaking change)
  • New feature (non-breaking change)
  • Breaking change (fix or feature with breaking changes)
  • Documentation update

Breaking Changes

Test Plan

  • Unit tests added/updated
  • Integration tests added/updated
  • Passed make installcheck
  • Passed make -C src/test installcheck-cbdb-parallel

Impact

Performance:

User-facing changes:

Dependencies:

Checklist

Additional Context

CI Skip Instructions



if (get_dirent_type(path, de, false, ERROR) != PGFILETYPE_LNK)
ereport(allow_in_place_tablespaces ? WARNING : PANIC,
ereport(allow_in_place_tablespaces ? WARNING : WARNING,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

PANIC is need to abort transcation.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This check is intended to restrict the internal tablespace to testing purposes only, but throwing a panic directly is too strict.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how about use ERROR?

* code that executes the command in't prepared for a NULL.
*/
if (conninfo == NULL)
conninfo = pstrdup("");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why not elog(ERROR) here ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

In this context, "" is considered a valid value. Therefore, we only need to convert the format.


my $last_nodetag = 'WindowObjectData';
my $last_nodetag_no = 557;
my $last_nodetag_no = 584;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how to decide last_nodetag_no?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Although gen_node_support.pl calculates this value automatically, manual modification is still required. I don't quite understand why it was designed this way.

@leborchukleborchuk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, let's clear the mess. We have 2 functions with the same name:

src/backend/nodes/queryjumblefuncs.c:JumbleQuery(Query *query)
src/backend/utils/misc/queryjumble.c:static void JumbleQueryInternal(JumbleState *jstate, Query *query);

Should use only one - those which was moved to the src/backend/nodes

@leborchuk
leborchuk requested a review from CopilotJuly 1, 2026 08:52
@leborchuk

Copy link
Copy Markdown
Contributor

It will be great to add good PR description there. I asked Copilot for review changes, one could get the base description from Copilot output

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors Cloudberry’s node infrastructure to generate query jumble and node support code automatically (including nodetag generation and updated node read/write/copy behavior), and updates regression expected files to match the new serialized node output.

Changes:

  • Move/modernize query jumble interfaces under src/include/nodes/ and switch callers to the updated API.
  • Extend gen_node_support.pl + build rules to generate/ship nodetags.h and additional generated node support outputs (including query-jumble support).
  • Update multiple node struct definitions/attributes and refresh regression expected outputs accordingly.

Reviewed changes

Copilot reviewed 47 out of 51 changed files in this pull request and generated 7 comments.

Show a summary per file
FileDescription
src/test/singlenode_regress/expected/partition.outUpdates expected partition template serialization output.
src/test/regress/expected/xml.outAdjusts expected XML serialization formatting.
src/test/regress/expected/partition_append.outUpdates expected partition template serialization output.
src/test/regress/expected/expressions.outUpdates expected output for NOT IN (...) NULL semantics.
src/include/utils/queryenvironment.hAdds node attribute annotation for generated support behavior.
src/include/parser/analyze.hSwitches queryjumble include to the new header path.
src/include/nodes/tidbitmap.hRefactors bitmap word array sizing constant.
src/include/nodes/queryjumble.hMoves/updates query jumble public header and API surface.
src/include/nodes/primnodes.hUpdates node attributes / renames anonymous structs.
src/include/nodes/plannodes.hAdds node attributes for generated copy/read/write behavior.
src/include/nodes/pathnodes.hAdds/adjusts node attributes for generated support behavior.
src/include/nodes/parsenodes.hAdds/adjusts node attributes and a few struct typedef names.
src/include/nodes/nodes.hReplaces large inline NodeTag enum with generated nodetags.h include.
src/include/nodes/altertablenodes.hAdds node attributes for generated support behavior.
src/include/MakefileInstalls generated nodes/nodetags.h; cleans generated node headers.
src/include/executor/execdesc.hAdds node attributes / renames anonymous struct typedef.
src/include/commands/explain_gp.hNew header extracting EXPLAIN GP structs.
src/include/catalog/heap.hAdds read/write-ignore defaults via node attributes.
src/include/access/tupdesc.hRenames typedef + adds node attributes.
src/backend/utils/misc/queryjumble.cRemoves legacy query jumble implementation.
src/backend/utils/misc/MakefileStops building the removed queryjumble.o.
src/backend/utils/misc/guc_tables.cUpdates includes to new queryjumble header location.
src/backend/postmaster/postmaster.cUpdates includes to new queryjumble header location.
src/backend/parser/analyze.cSwitches to new JumbleQuery() call signature.
src/backend/nodes/read.cChanges error severity for token parsing failure.
src/backend/nodes/nodeFuncs.cAdjusts mutator behavior for certain node tags.
src/backend/nodes/MakefileAdds generated-header rules and new node support objects.
src/backend/nodes/gen_node_support.plMajor generator extensions (out/read fast + queryjumble generation).
src/backend/nodes/copyfuncs.switch.cRemoves generated switch file (now regenerated differently).
src/backend/nodes/copyfuncs.cAdjusts pointer-copy macro + adds custom copy routines.
src/backend/nodes/.gitignoreIgnores newly generated node-support artifacts.
src/backend/MakefileRe-enables node header generation step.
src/backend/executor/execProcnode.cRemoves references to a no-longer-present planstate tag.
src/backend/executor/execAmi.cRemoves references to a no-longer-present planstate tag.
src/backend/commands/subscriptioncmds.cNormalizes conninfo to avoid NULL vs empty-string ambiguity.
src/backend/commands/explain.cSwitches to new JumbleQuery() call signature.
src/backend/commands/explain_gp.cUses new extracted header for EXPLAIN GP stats structs.
src/backend/access/transam/xlogrecovery.cChanges severity handling for unexpected pg_tblspc/ entries.
gpcontrib/pg_hint_plan/pg_hint_plan.cSwitches to new JumbleQuery() call signature.
gpcontrib/gp_stats_collector/src/stat_statements_parser/pg_stat_statements_parser.cUpdates includes to new queryjumble header location.
contrib/pax_storage/src/test/regress/expected/xml.outMirrors XML expected-output change for pax_storage tests.
contrib/pax_storage/src/test/regress/expected/partition.outMirrors partition expected-output change for pax_storage tests.
contrib/pax_storage/src/test/regress/expected/partition_optimizer.outMirrors partition optimizer expected-output change for pax_storage tests.
contrib/pax_storage/src/test/regress/expected/expressions.outMirrors expressions expected-output change for pax_storage tests.
contrib/pax_storage/src/test/regress/expected/bfv_partition.outMirrors partition template serialization changes for BFV output.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1454 to +1461
elsif($t eq 'bytea*')
{
print $ofaf
"\tWRITE_BYTEA_FIELD($f);\n";
print $rfaf
"\tREAD_BYTEA_FIELD($f);\n"
unless $no_read;
}
Comment on lines +1518 to +1524
if (defined $read_as_field)
{
print $rff "\tlocal_node->$f = $read_as_field;\n" unless $no_read;

# for out and read fast
print $rff "\tlocal_node->$f = $read_as_field;\n" unless $no_read;
}
if (jumblequery)
{
jstate = JumbleQueryDirect(query, query_str);
jstate = JumbleQuery(query);
/*
* conninfo can be an empty string, but the serialization
* doesn't distinguish an empty string from NULL. The
* code that executes the command in't prepared for a NULL.
else
{
elog(ERROR, "unrecognized token: \"%.*s\"", tok_len, token);
elog(PANIC, "unrecognized token: \"%.*s\"", tok_len, token);

if (get_dirent_type(path, de, false, ERROR) != PGFILETYPE_LNK)
ereport(allow_in_place_tablespaces ? WARNING : PANIC,
ereport(allow_in_place_tablespaces ? WARNING : WARNING,
Comment on lines +21 to +22


@tuhaihe

Copy link
Copy Markdown
Member

Hi, how about the progress? Could we start to resolve the comments and try to merge this PR for PG16 FIXME?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

5 participants

@chenjinbao1989@leborchuk@tuhaihe@yjhjstz