fix: serialize retvarno field in PLpgSQL RETURN/RETURN NEXT JSON output - #333
Open
pyramation wants to merge 2 commits into
Open
fix: serialize retvarno field in PLpgSQL RETURN/RETURN NEXT JSON output#333pyramation wants to merge 2 commits into
retvarno field in PLpgSQL RETURN/RETURN NEXT JSON output#333pyramation wants to merge 2 commits into
Conversation
The PLpgSQL_stmt_return and PLpgSQL_stmt_return_next nodes have a retvarno field that references the datum index of the variable being returned. This field is populated by the parser but not serialized to JSON because WRITE_INT_FIELD(retvarno) is commented out in pg_query_json_plpgsql.c. This test will FAIL until the serializer is fixed, proving the bug.
Uncomment WRITE_INT_FIELD(retvarno) in dump_return() and dump_return_next() with the correct 3-argument macro syntax. The retvarno field is an index into the datums array that identifies which variable is being returned. It was populated by the parser but never serialized to JSON, making it impossible for downstream deparsers to recover the variable name in RETURN NEXT statements.
Member
|
@pyramation Thanks for the PR! I think this makes sense, but the way I'd like to fix this is to auto-generate the output functions (#323), which is on my short-list for a follow-up release after the Postgres 18 release (which I just tagged, finally). |
Contributor
Author
|
cool sounds good! whatever is best :) |
Contributor
Author
|
and btw, thanks for the Postgres 18 release! Just released the the first downstream package https://github.com/constructive-io/libpg-query-node and now going to continue to https://github.com/constructive-io/pgsql-parser for the full toolset :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
PLpgSQL_stmt_returnandPLpgSQL_stmt_return_nextC structs have aretvarnofield — an index into thedatumsarray identifying which variable is being returned. The parser populates this field, but the JSON serializer inpg_query_json_plpgsql.chad it commented out://WRITE_INT_FIELD(retvarno);This made it impossible for downstream deparsers (e.g. pgsql-parser) to recover the variable name in
RETURN NEXT variablestatements. Withoutretvarno, a deparser sees:{ "PLpgSQL_stmt_return_next": { "lineno": 9 } }…and has no way to know that the original SQL was
RETURN NEXT r;vsRETURN NEXT v_count;. The datums array lists all declared variables, but withoutretvarnothere's no pointer from the RETURN NEXT statement to the specific datum.Fix
Uncomment
WRITE_INT_FIELDforretvarnoin bothdump_return()anddump_return_next(), using the correct 3-argument macro syntax:The original commented-out line only had 1 argument (
//WRITE_INT_FIELD(retvarno)) which would have been a compile error — the macro requires(outname, outname_json, fldname).Output values
retvarnovalue≥ 0datums[]for the returned variableRETURN NEXT r;→retvarno: 1→datums[1].refname = "r"-1exprfield insteadRETURN v_name || '/' || v_version;Note:
WRITE_INT_FIELDhas a!= 0guard, soretvarno: 0would not be serialized. In practice this is fine — datum 0 is always the auto-createdfoundboolean, which is never the target of RETURN/RETURN NEXT.Backward compatibility
retvarno >= 0before using it as a datum index.Commits
test: expected JSON should include retvarno for RETURN/RETURN NEXT— updatesplpgsql_samples.expected.jsonto includeretvarnovalues. This test FAILS on17-latest(proving the bug).fix: serialize retvarno field in PLpgSQL RETURN/RETURN NEXT JSON output— uncomments the serializer. All tests pass.