Thread
Commits
-
Fix incorrect declarations of variadic pg_get_*_ddl() functions.
- 2af70e937478 19 (unreleased) landed
-
Fix variadic argument types for pg_get_xxx_ddl() functions
Chao Li <li.evan.chao@gmail.com> — 2026-06-23T02:11:52Z
Hi, While testing "[76e514ebb] Add pg_get_role_ddl() function" as well as the other pg_get_xxx_ddl() functions, I noticed that their pg_proc.dat definitions use "text" rather than "_text" for variadic argument types. For example: ``` { oid => '6501', descr => 'get DDL to recreate a role', proname => 'pg_get_role_ddl', prorows => '10', provariadic => 'text', proisstrict => 'f', proretset => 't', provolatile => 's', pronargdefaults => '1', prorettype => 'text', proargtypes => 'regrole text', proallargtypes => '{regrole,text}', proargmodes => '{i,v}', proargdefaults => '{NULL}', prosrc => 'pg_get_role_ddl' }, ``` I spotted this problem some time ago, but I wasn’t sure whether it was a real issue worth fixing. When I saw a new patch [1] for pg_get_table_ddl() that had the same problem, I asked about it there. Akshay confirmed the problem and pointed me to the opr_sanity check. Thank you very much, Akshay. Then I checked why the sanity check didn’t report the issue. I found that the check used != for the comparison. When the final argument type is not an array type, the subquery returns NULL, and NULL != provariadic evaluates to NULL rather than true. I fixed the check to use IS DISTINCT FROM. After that, it reported the mismatch: ``` % make -C src/test/regress check-tests TESTS='test_setup opr_sanity' # +++ regress check in src/test/regress +++ # initializing database system by copying initdb template # using temp instance on port 58928 with PID 36865 ok 1 - test_setup 243 ms # diff -U3 /Users/chaol/Documents/code/postgresql/src/test/regress/expected/opr_sanity.out /Users/chaol/Documents/code/postgresql/src/test/regress/results/opr_sanity.out # --- /Users/chaol/Documents/code/postgresql/src/test/regress/expected/opr_sanity.out 2026-06-23 07:58:52 # +++ /Users/chaol/Documents/code/postgresql/src/test/regress/results/opr_sanity.out 2026-06-23 07:59:41 # @@ -488,9 +488,13 @@ # FROM pg_type t # WHERE t.typarray = proargtypes[array_length(proargtypes, 1)-1]) # END IS DISTINCT FROM provariadic; # - oid | provariadic | proargtypes # ------+-------------+------------- # -(0 rows) # + oid | provariadic | proargtypes # +---------------------------------------+-------------+-------------------------- # + pg_get_role_ddl(regrole,text) | text | [0:1]={regrole,text} # + pg_get_tablespace_ddl(oid,text) | text | [0:1]={oid,text} # + pg_get_tablespace_ddl(name,text) | text | [0:1]={name,text} # + pg_get_database_ddl(regdatabase,text) | text | [0:1]={regdatabase,text} # +(4 rows) # # -- Check that all and only those functions with a variadic type have # -- a variadic argument. not ok 2 - opr_sanity 8938 ms 1..2 # 1 of 2 tests failed. ``` After updating pg_proc.dat to change those "text" entries to "_text", the test passed. See the attached patch for details. [1] https://postgr.es/m/CANxoLDe_GOXyyZs7GN3VUJoT+o1DwqZGhid-TWWvS0D8d5ggYw@mail.gmail.com Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: Fix variadic argument types for pg_get_xxx_ddl() functions
Chao Li <li.evan.chao@gmail.com> — 2026-06-23T08:17:37Z
> On Jun 23, 2026, at 10:11, Chao Li <li.evan.chao@gmail.com> wrote: > > Hi, > > While testing "[76e514ebb] Add pg_get_role_ddl() function" as well as the other pg_get_xxx_ddl() functions, I noticed that their pg_proc.dat definitions use "text" rather than "_text" for variadic argument types. For example: > ``` > { oid => '6501', descr => 'get DDL to recreate a role', > proname => 'pg_get_role_ddl', prorows => '10', provariadic => 'text', > proisstrict => 'f', proretset => 't', provolatile => 's', > pronargdefaults => '1', prorettype => 'text', proargtypes => 'regrole text', > proallargtypes => '{regrole,text}', proargmodes => '{i,v}', > proargdefaults => '{NULL}', prosrc => 'pg_get_role_ddl' }, > ``` > > I spotted this problem some time ago, but I wasn’t sure whether it was a real issue worth fixing. When I saw a new patch [1] for pg_get_table_ddl() that had the same problem, I asked about it there. Akshay confirmed the problem and pointed me to the opr_sanity check. Thank you very much, Akshay. > > Then I checked why the sanity check didn’t report the issue. I found that the check used != for the comparison. When the final argument type is not an array type, the subquery returns NULL, and NULL != provariadic evaluates to NULL rather than true. I fixed the check to use IS DISTINCT FROM. After that, it reported the mismatch: > ``` > % make -C src/test/regress check-tests TESTS='test_setup opr_sanity' > # +++ regress check in src/test/regress +++ > # initializing database system by copying initdb template > # using temp instance on port 58928 with PID 36865 > ok 1 - test_setup 243 ms > # diff -U3 /Users/chaol/Documents/code/postgresql/src/test/regress/expected/opr_sanity.out /Users/chaol/Documents/code/postgresql/src/test/regress/results/opr_sanity.out > # --- /Users/chaol/Documents/code/postgresql/src/test/regress/expected/opr_sanity.out 2026-06-23 07:58:52 > # +++ /Users/chaol/Documents/code/postgresql/src/test/regress/results/opr_sanity.out 2026-06-23 07:59:41 > # @@ -488,9 +488,13 @@ > # FROM pg_type t > # WHERE t.typarray = proargtypes[array_length(proargtypes, 1)-1]) > # END IS DISTINCT FROM provariadic; > # - oid | provariadic | proargtypes > # ------+-------------+------------- > # -(0 rows) > # + oid | provariadic | proargtypes > # +---------------------------------------+-------------+-------------------------- > # + pg_get_role_ddl(regrole,text) | text | [0:1]={regrole,text} > # + pg_get_tablespace_ddl(oid,text) | text | [0:1]={oid,text} > # + pg_get_tablespace_ddl(name,text) | text | [0:1]={name,text} > # + pg_get_database_ddl(regdatabase,text) | text | [0:1]={regdatabase,text} > # +(4 rows) > # > # -- Check that all and only those functions with a variadic type have > # -- a variadic argument. > not ok 2 - opr_sanity 8938 ms > 1..2 > # 1 of 2 tests failed. > ``` > > After updating pg_proc.dat to change those "text" entries to "_text", the test passed. > > See the attached patch for details. > > [1] https://postgr.es/m/CANxoLDe_GOXyyZs7GN3VUJoT+o1DwqZGhid-TWWvS0D8d5ggYw@mail.gmail.com > > Best regards, > -- > Chao Li (Evan) > HighGo Software Co., Ltd. > https://www.highgo.com/ > > > > Oops! Missed the attachment. -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: Fix variadic argument types for pg_get_xxx_ddl() functions
Akshay Joshi <akshay.joshi@enterprisedb.com> — 2026-06-23T08:34:42Z
Hi Chao, Patch looks good to me. I have added the following test cases in my local version in "*src/test/modules/test_misc/t/012_ddlutils.pl <http://012_ddlutils.pl>*". If possible add those and resend the patch +# Options passed via VARIADIC ARRAY +$result = $node->safe_psql('postgres', + q{SELECT * FROM pg_get_role_ddl('regress_role_ddl_test2', + VARIADIC ARRAY['pretty', 'false'])}); +unlike($result, qr/\n\s+SUPERUSER/, + 'role VARIADIC ARRAY pretty=false suppresses indentation'); +like($result, qr/SUPERUSER/, 'role VARIADIC ARRAY pretty=false still emits attributes'); +# Options passed via VARIADIC ARRAY +$result = ddl_filter( + $node->safe_psql( + 'postgres', + q{SELECT pg_get_database_ddl + FROM pg_get_database_ddl('regression_ddlutils_test', + VARIADIC ARRAY['pretty', 'false'])})); +unlike($result, qr/\n\s+WITH TEMPLATE/, + 'database VARIADIC ARRAY pretty=false suppresses indentation'); +like( + $result, + qr/CREATE DATABASE regression_ddlutils_test/, + 'database VARIADIC ARRAY still emits CREATE'); +# Options passed via VARIADIC ARRAY (name variant) +$result = $node->safe_psql( + 'postgres', + q{SELECT * FROM pg_get_tablespace_ddl('regress_allopt_tblsp', + VARIADIC ARRAY['pretty', 'false'])}); +unlike($result, qr/\n\s+OWNER/, + 'tablespace VARIADIC ARRAY pretty=false suppresses indentation (name)'); +like( + $result, + qr/OWNER regress_role_ddl_test1/, + 'tablespace VARIADIC ARRAY still emits OWNER (name)'); + +# Options passed via VARIADIC ARRAY (oid variant) +$result = $node->safe_psql( + 'postgres', q{ + SELECT * FROM pg_get_tablespace_ddl( + (SELECT oid FROM pg_tablespace + WHERE spcname = 'regress_allopt_tblsp'), + VARIADIC ARRAY['pretty', 'false'])}); +unlike($result, qr/\n\s+OWNER/, + 'tablespace VARIADIC ARRAY pretty=false suppresses indentation (oid)'); +like( + $result, + qr/CREATE TABLESPACE regress_allopt_tblsp/, + 'tablespace VARIADIC ARRAY still emits CREATE (oid)'); + On Tue, Jun 23, 2026 at 1:50 PM Chao Li <li.evan.chao@gmail.com> wrote: > > > > On Jun 23, 2026, at 10:11, Chao Li <li.evan.chao@gmail.com> wrote: > > > > Hi, > > > > While testing "[76e514ebb] Add pg_get_role_ddl() function" as well as > the other pg_get_xxx_ddl() functions, I noticed that their pg_proc.dat > definitions use "text" rather than "_text" for variadic argument types. For > example: > > ``` > > { oid => '6501', descr => 'get DDL to recreate a role', > > proname => 'pg_get_role_ddl', prorows => '10', provariadic => 'text', > > proisstrict => 'f', proretset => 't', provolatile => 's', > > pronargdefaults => '1', prorettype => 'text', proargtypes => 'regrole > text', > > proallargtypes => '{regrole,text}', proargmodes => '{i,v}', > > proargdefaults => '{NULL}', prosrc => 'pg_get_role_ddl' }, > > ``` > > > > I spotted this problem some time ago, but I wasn’t sure whether it was a > real issue worth fixing. When I saw a new patch [1] for pg_get_table_ddl() > that had the same problem, I asked about it there. Akshay confirmed the > problem and pointed me to the opr_sanity check. Thank you very much, Akshay. > > > > Then I checked why the sanity check didn’t report the issue. I found > that the check used != for the comparison. When the final argument type is > not an array type, the subquery returns NULL, and NULL != provariadic > evaluates to NULL rather than true. I fixed the check to use IS DISTINCT > FROM. After that, it reported the mismatch: > > ``` > > % make -C src/test/regress check-tests TESTS='test_setup opr_sanity' > > # +++ regress check in src/test/regress +++ > > # initializing database system by copying initdb template > > # using temp instance on port 58928 with PID 36865 > > ok 1 - test_setup 243 ms > > # diff -U3 > /Users/chaol/Documents/code/postgresql/src/test/regress/expected/opr_sanity.out > /Users/chaol/Documents/code/postgresql/src/test/regress/results/opr_sanity.out > > # --- > /Users/chaol/Documents/code/postgresql/src/test/regress/expected/opr_sanity.out > 2026-06-23 07:58:52 > > # +++ > /Users/chaol/Documents/code/postgresql/src/test/regress/results/opr_sanity.out > 2026-06-23 07:59:41 > > # @@ -488,9 +488,13 @@ > > # FROM pg_type t > > # WHERE t.typarray = > proargtypes[array_length(proargtypes, 1)-1]) > > # END IS DISTINCT FROM provariadic; > > # - oid | provariadic | proargtypes > > # ------+-------------+------------- > > # -(0 rows) > > # + oid | provariadic | > proargtypes > > # > +---------------------------------------+-------------+-------------------------- > > # + pg_get_role_ddl(regrole,text) | text | > [0:1]={regrole,text} > > # + pg_get_tablespace_ddl(oid,text) | text | > [0:1]={oid,text} > > # + pg_get_tablespace_ddl(name,text) | text | > [0:1]={name,text} > > # + pg_get_database_ddl(regdatabase,text) | text | > [0:1]={regdatabase,text} > > # +(4 rows) > > # > > # -- Check that all and only those functions with a variadic type have > > # -- a variadic argument. > > not ok 2 - opr_sanity 8938 ms > > 1..2 > > # 1 of 2 tests failed. > > ``` > > > > After updating pg_proc.dat to change those "text" entries to "_text", > the test passed. > > > > See the attached patch for details. > > > > [1] > https://postgr.es/m/CANxoLDe_GOXyyZs7GN3VUJoT+o1DwqZGhid-TWWvS0D8d5ggYw@mail.gmail.com > > > > Best regards, > > -- > > Chao Li (Evan) > > HighGo Software Co., Ltd. > > https://www.highgo.com/ > > > > > > > > > > Oops! Missed the attachment. > > -- > Chao Li (Evan) > HighGo Software Co., Ltd. > https://www.highgo.com/ > > > > > -
Re: Fix variadic argument types for pg_get_xxx_ddl() functions
Chao Li <li.evan.chao@gmail.com> — 2026-06-23T09:29:04Z
> On Jun 23, 2026, at 16:34, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote: > > Hi Chao, > > Patch looks good to me. I have added the following test cases in my local version in "src/test/modules/test_misc/t/012_ddlutils.pl". If possible add those and resend the patch > > +# Options passed via VARIADIC ARRAY > +$result = $node->safe_psql('postgres', > + q{SELECT * FROM pg_get_role_ddl('regress_role_ddl_test2', > + VARIADIC ARRAY['pretty', 'false'])}); > +unlike($result, qr/\n\s+SUPERUSER/, > + 'role VARIADIC ARRAY pretty=false suppresses indentation'); > +like($result, qr/SUPERUSER/, 'role VARIADIC ARRAY pretty=false still emits attributes'); > > > +# Options passed via VARIADIC ARRAY > +$result = ddl_filter( > + $node->safe_psql( > + 'postgres', > + q{SELECT pg_get_database_ddl > + FROM pg_get_database_ddl('regression_ddlutils_test', > + VARIADIC ARRAY['pretty', 'false'])})); > +unlike($result, qr/\n\s+WITH TEMPLATE/, > + 'database VARIADIC ARRAY pretty=false suppresses indentation'); > +like( > + $result, > + qr/CREATE DATABASE regression_ddlutils_test/, > + 'database VARIADIC ARRAY still emits CREATE'); > > > +# Options passed via VARIADIC ARRAY (name variant) > +$result = $node->safe_psql( > + 'postgres', > + q{SELECT * FROM pg_get_tablespace_ddl('regress_allopt_tblsp', > + VARIADIC ARRAY['pretty', 'false'])}); > +unlike($result, qr/\n\s+OWNER/, > + 'tablespace VARIADIC ARRAY pretty=false suppresses indentation (name)'); > +like( > + $result, > + qr/OWNER regress_role_ddl_test1/, > + 'tablespace VARIADIC ARRAY still emits OWNER (name)'); > + > +# Options passed via VARIADIC ARRAY (oid variant) > +$result = $node->safe_psql( > + 'postgres', q{ > + SELECT * FROM pg_get_tablespace_ddl( > + (SELECT oid FROM pg_tablespace > + WHERE spcname = 'regress_allopt_tblsp'), > + VARIADIC ARRAY['pretty', 'false'])}); > +unlike($result, qr/\n\s+OWNER/, > + 'tablespace VARIADIC ARRAY pretty=false suppresses indentation (oid)'); > +like( > + $result, > + qr/CREATE TABLESPACE regress_allopt_tblsp/, > + 'tablespace VARIADIC ARRAY still emits CREATE (oid)'); > + > Hi Akshay, I actually considered adding VARIADIC ARRAY test cases when I worked on the patch, but I intentionally avoided them because I found that similar variadic functions, such as json_extract_path and jsonb_extract_path, don’t have such tests either. Also, the sanity check is now able to detect this problem. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: Fix variadic argument types for pg_get_xxx_ddl() functions
Tom Lane <tgl@sss.pgh.pa.us> — 2026-06-23T18:47:29Z
Chao Li <li.evan.chao@gmail.com> writes: > On Jun 23, 2026, at 10:11, Chao Li <li.evan.chao@gmail.com> wrote: >> Then I checked why the sanity check didn’t report the issue. I found that the check used != for the comparison. When the final argument type is not an array type, the subquery returns NULL, and NULL != provariadic evaluates to NULL rather than true. I fixed the check to use IS DISTINCT FROM. After that, it reported the mismatch: Ouch. Yes, that regression test is clearly one brick shy of a load. We need to update the documentation for these functions too, and maybe some other places. regards, tom lane
-
Re: Fix variadic argument types for pg_get_xxx_ddl() functions
Tom Lane <tgl@sss.pgh.pa.us> — 2026-06-23T19:07:48Z
I wrote: > Ouch. Yes, that regression test is clearly one brick shy of a load. > We need to update the documentation for these functions too, and maybe > some other places. After looking around, I couldn't find anything but the documentation that needed to be fixed, so done. regards, tom lane
-
Re: Fix variadic argument types for pg_get_xxx_ddl() functions
Andrew Dunstan <andrew@dunslane.net> — 2026-06-23T19:17:52Z
On Tue, Jun 23, 2026 at 3:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I wrote: > > Ouch. Yes, that regression test is clearly one brick shy of a load. > > We need to update the documentation for these functions too, and maybe > > some other places. > > After looking around, I couldn't find anything but the documentation > that needed to be fixed, so done. > > Thanks for fixing. cheers andrew