Re: Add pg_get_publication_ddl function

Some review comments for patch v3-0001.

======
doc/src/sgml/func/func-info.sgml

1.
+ <para role="func_signature">
+ <function>pg_get_publication_ddl</function>
+ ( <parameter>publication</parameter> <type>text</type>

Why is the first parameter type 'text' instead of 'name' like
pg_get_tablespace_ddl was?

~~~

2.
+ <para>
+ Reconstructs the <link
linkend="sql-createpublication"><command>CREATE
PUBLICATION</command></link> statement for
+ the specified publication (by OID or name), followed by an
+ <command>ALTER PUBLICATION ... OWNER TO</command> statement (the
+ <command>CREATE PUBLICATION</command> grammar has no
+ <literal>OWNER</literal> clause). Each statement is returned as a
+ separate row. An error is raised if no publication with the supplied
+ OID or name exists.
+ The following options are supported:
+ <literal>pretty</literal> (boolean) for formatted output and
+ <literal>owner</literal> (boolean) to include
+ <literal>OWNER</literal>.
+ </para></entry>

Thanks for adding the link to CREATE. Consider doing the same for the
ALTER ... OWNER.

======
src/backend/utils/adt/ddlutils.c

3.
+static List *pg_get_publication_ddl_internal(Oid puboid, bool pretty,
+ bool no_owner);

The `no_owner` parameter is backwards. I suppose it's copying an
existing pattern, but that doesn't make it right. IMO this parameter
ought to have the same positive/negative polarity as per the
documentation.

So, the parameter should be 'owner' or 'include_owner'. It avoids
mental gymnastics to read, and ! operations in the function and
caller.

e.g.
BEFORE
+ /* OWNER */
+ if (!no_owner)

SUGGESTION
/* OWNER */
if (owner)

~~~

pg_get_publication_ddl_internal:

> > 7.
> > + pubtuple = SearchSysCache2(PUBLICATIONRELMAP, ObjectIdGetDatum(relid),
> > + ObjectIdGetDatum(pub->oid));
> > +
> > + if (!HeapTupleIsValid(pubtuple))
> > + elog(ERROR,
> > + "cache lookup failed for publication relation %u in publication %u",
> > + relid, pub->oid);
> >
> > 7a.
> > Maybe blank line here is not wanted.
>
> It gives some space, it was intentional.

REPLY: Fine, but it's not consistent with the 'reltup' immediate prior this.

> > 10.
> > + /*
> > + * If there is a condition it goes after the columns. We can have
> > + * conditions without columns as well.
> > + */
> > + if (!condition_nulls)
> >
> > 10a.
> > The earlier assignment to 'conditions' should be moved to be directly
> > above here.
>
> In this case I think it's better to have both calls to SysCacheGetAttr()
> together since both depends on the previous call of SearchSysCache2()

REPLY: Moving the code also has the advantage that then you can just
use a common variables like `datum` and `isnull` as per Alvaro's
review [1].

> > 10b.
> > BTW, it is called a "row filter" so maybe it is better to refer to
> > that in the comments/vars instead of the generic sounding "condition".
>
> Changed!

REPLY: The accompanying `condition_nulls` was not changed.

~~~

4.
+ if (!condition_nulls)
+ {
+ Node *node;
+ List *context;
+ char *str;
+
+ node = stringToNode(TextDatumGetCString(rowfilter));
+ context = deparse_context_for(tablename, relid);
+ str = deparse_expression(node, context, false, false);
+ appendStringInfo(&buf, " WHERE (%s)", str);
+ }

This is causing double parentheses in the WHERE. See the test output:
+ CREATE PUBLICATION testpub_ddl_4
+
+ FOR TABLE public.testpub_ddl_tbl3(bar, baz) WHERE ((bar = baz))
+
+ WITH (publish='insert, update, delete, truncate',
publish_generated_columns=none, publish_via_partition_root=false);

Can it be fixed to add parentheses only when it's necessary? e.g. I
see some code in ruleutil.c that is checking to add parens or not --
perhaps you need something similar.

Furthermore, I saw ruleutil.c has a 'deparse_expression_pretty'.
Shouldn't we be trying to pass the 'pretty' flag into that? AFAICT the
'deparse_expression' always says pretty is false.

~~~

5.
+ /* If we have schemas, they will go right before the EXCEPT and/or WITH */

In PG19, EXCEPT is only allowed with FOR ALL TABLES. Furthermore, FOR
ALL TABLES and FOR TABLES IN SCHEMA cannot coexist. So, mentioning
EXCEPT in this comment about schemas seems meaningless. Schema won't
go right before an EXCEPT because schemas cannot (yet) coexist with
EXCEPT.

~~~

6.
+ /* Always add the WITH options */
+ append_ddl_option(&buf, pretty, 4, "WITH (");
+
+ /* Publish string */
+ appendStringInfoString(&buf, "publish='");
+
+
+ if (pub->pubactions.pubinsert)

Double spacing?

======
src/include/catalog/pg_proc.dat

7.
+ proargtypes => 'text text',
+ proargmodes => '{i,v}',
+ proallargtypes => '{text,text}',
+ pronargdefaults => '1', proargdefaults => '{NULL}',
+ prosrc => 'pg_get_publication_ddl_name' },

Shouldn't this be 'name text', same as "pg_get_tablespace_ddl_name"?

======
src/test/regress/sql/publication_ddl.sql

8.
According to the documentation the output of ALTER ... OWNER depends
on the 'owner' parameter. AFAICT every test here is passing 'owner' as
true.

e.g
+SELECT pg_get_publication_ddl('testpub_ddl_1', 'pretty', 'true');

This results in more work and more output than is necessary. Probably,
there only needs to be only 1 test passing owner 'true', and every
other test case can pass it 'false'.

~~~

9.
+-- create publication for one table with two columns and a rowfilter
+CREATE PUBLICATION testpub_ddl_4 FOR TABLE ONLY testpub_ddl_tbl3
(bar,baz) WHERE (bar = baz);
+
+SELECT pg_get_publication_ddl('testpub_ddl_4');
+SELECT pg_get_publication_ddl((SELECT oid FROM pg_publication WHERE
pubname='testpub_ddl_4'));
+SELECT pg_get_publication_ddl('testpub_ddl_4', 'pretty', 'true');
+
+-- create publication for one table with two columns and a rowfilter
+CREATE PUBLICATION testpub_ddl_5 FOR TABLE ONLY testpub_ddl_tbl4
(bar,beque) WHERE (beque IS TRUE);
+
+SELECT pg_get_publication_ddl('testpub_ddl_5');
+SELECT pg_get_publication_ddl((SELECT oid FROM pg_publication WHERE
pubname='testpub_ddl_5'));
+SELECT pg_get_publication_ddl('testpub_ddl_5', 'pretty', 'true');
+

9a.
There are 2 test case here with exactly the same comment

~

9b.
/rowfilter/row filter/

~~~

10.
+-- create a publication with a bare bolean in the row filter
+CREATE PUBLICATION testpub_ddl_10 FOR TABLE testpub_ddl_tbl4 WHERE (beque);

/bolean/boolean/

======
[1] Alvaro - https://www.postgresql.org/message-id/aihkCoNg3evwHYUj%40alvherre.pgsql

Kind Regards,
Peter Smith.
Fujitsu Australia