Re: Add pg_get_publication_ddl function

Hi.

Some review comments for the v2-0001 patch.

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

(9.28.13. Get Object DDL Functions)

1.
+ <para role="func_signature">
+ <function>pg_get_publication_ddl</function>
+ ( <parameter>publication</parameter> <type>text</type>
+ <optional>, <literal>VARIADIC</literal> <parameter>options</parameter>
+ <type>text</type> </optional> )
+ <returnvalue>setof text</returnvalue>
+ </para>

I think "pubname" might be a more meaningful name for the first parameter.

~~~

2.
+ <para>
+ Reconstructs the <command>CREATE PUBLICATION</command> 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. When the publication was created with
+ <literal>FOR ALL TABLES, ALL SEQUENCES</literal>, the emitted
+ statement always lists <literal>ALL TABLES</literal> before
+ <literal>ALL SEQUENCES</literal> regardless of the original order.
+ The following options are supported:
+ <literal>pretty</literal> (boolean) for formatted output and
+ <literal>owner</literal> (boolean) to include
+ <literal>OWNER</literal>.
+ </para></entry>

2a.
That "CREATE PUBLICATION" should <link> back to the CREATE PUBLICATION
docs page.

~

2b.
It is overkill to mention about the potential reordering of ALL TABLES
and ALL SEQUENCES.

Apart from being unnecessary, there are many other things can also be
rearranged which are not mentioned:
- TABLES and ALL TABLES IN SCHEMA clauses might be different order
than specified
- The publication parameters might be in a different order than specified
- The values of 'publish' parameter might be different order than specified
- etc.

~~~

GENERAL

3.
It would be better if the the rows of "Table 9.96" were in alphabetical order.

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

pg_get_publication_ddl_internal:

4.
+ if (pub->allsequences)
+ appendStringInfo(buf,
+ "%sALL SEQUENCES",
+ pub->alltables ? ", " : "");

Maybe better to avoid tricky format strings.

SUGGESTION
if (pub->allsequences)
{
if (pub->alltables)
appendStringInfo(buf, ", ");

appendStringInfo("ALL SEQUENCES");
}

~~~

5.
+ if (pub_incl_relids != NIL)
+ {
+ ListCell *pub_cell;
+ char *schemaname = NULL;
+ char *tablename;
+
+ append_ddl_option(buf, pretty, 4, "FOR TABLE ");
+
+ /*
+ * Publication can have table relations
+ */
+ foreach(pub_cell, pub_incl_relids)

Maybe that comment belongs earlier (above the if).

~~~

6.
+ appendStringInfo(buf, "%s%s",
+ foreach_current_index(pub_cell) > 0 ? ", " : "",
+ quote_qualified_identifier(schemaname, tablename));

Another place where avoiding a tricky format string may be tidier.

SUGGESTION
if (foreach_current_index(pub_cell) > 0)
appendStringInfo(buf, ", ");

appendStringInfo(buf, "%s", quote_qualified_identifier(schemaname, tablename));

~~~

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.

~

7b.
Don't need to say "publication" 2x.

/publication relation/relation/

~~~

8.
+ /* If non-null, we have a list of columns to publish */
+ if (!cols_nulls)

SUGGESTION FOR THE COMMENT
Does this table specify a column-list?

~~~

9.
+ appendStringInfo(buf, "%s%s",
+ bms_member_index(attmap, attnum) ? ", " : "",
+ quote_identifier(get_attname(relid, attnum, true)));

Another place where avoiding a tricky format string may be tidier.

(change similar to previous review comments)

~~~

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.

~

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".

~~~

11.
+ /* If we have schemas, they will go right before the WITH */

The kind of comments that just say "this-goes-after-that" or
"this-goes-after-that" are not very useful, because it is obvious from
the code logic that some appendStringInfo comes before or after
another one.

~~~

12.
+ /*
+ * Schemas can be preceded by a list of tables. When they are, the
+ * "TABLES IN SCHEMA" stays inline as a continuation of the existing
+ * FOR clause; otherwise it starts the FOR clause on its own line in
+ * pretty mode.
+ */

IMO it would be better for the FOR TABLE IN SCHEMA to come *before*
the specific tables in FOR TABLE.

e.g. For the case when there are specified tables "absorbed" into the
same named schemas I think it is more natural to see the schemas
first.
CREATE PUBLICATION mypub FOR TABLES IN SCHEMA s, TABLE s.t1;

~~~

13.
+ appendStringInfo(buf, "%s %s",
+ foreach_current_index(schema_cell) > 0 ? "," : "",
+ quote_identifier(nspname));

Another place where avoiding a tricky format string may be tidier.

(change similar to previous review comments)

~~~

14.
+ if (pub_excl_relids != NIL)
+ {
+ ListCell *excl_cell;
+ char *schemaname = NULL;
+
+ appendStringInfoString(buf, " EXCEPT (TABLE ");

The EXCEPT clause is currently permitted only with FOR ALL TABLES, so
it would be better moving this to earlier in this function where
pub->alltables was handled.

~~~

15.
+ appendStringInfo(buf, "%s%s",
+ foreach_current_index(excl_cell) > 0 ? ", " : "",
+ quote_qualified_identifier(schemaname, NameStr(reltup->relname)));

Another place where avoiding a tricky format string may be tidier.

(change similar to previous review comments)

~~~

16.
+ /*
+ * We need to know if we're the second permission added to prefix with a
+ * ", " string
+ */
+ if (pub->pubactions.pubinsert)
+ {
+ /*
+ * By precedence we know that the insert will always be first, no need
+ * to check previous values
+ */
+ appendStringInfoString(buf, "insert");

Both these comments are doing little more than just saying the same as
the code. IMO they are not needed.

~~~

17.
+ if (pub->pubactions.pubinsert)
+ {
+ /*
+ * By precedence we know that the insert will always be first, no need
+ * to check previous values
+ */
+ appendStringInfoString(buf, "insert");
+ first_perm = false;
+ }
+
+ if (pub->pubactions.pubupdate)
+ {
+ appendStringInfo(buf, "%supdate", first_perm ? "" : ", ");
+ first_perm = false;
+ }
+ if (pub->pubactions.pubdelete)
+ {
+ appendStringInfo(buf, "%sdelete", first_perm ? "" : ", ");
+ first_perm = false;
+ }
+
+ if (pub->pubactions.pubtruncate)
+ {
+ appendStringInfo(buf, "%struncate", first_perm ? "" : ", ");
+ }
+

17a.
There are some random blank lines that seem unnecessary.

~

17b.
IMO it is tidier to simply append the string you want, instead of
using a trick format string.

SUGGESTION (compare with patch)

if (pub->pubactions.pubinsert)
{
appendStringInfoString(buf, "insert");
first_perm = false;
}
if (pub->pubactions.pubupdate)
{
appendStringInfo(buf, first_perm ? "update" : ",update");
first_perm = false;
}
if (pub->pubactions.pubdelete)
{
appendStringInfo(buf, first_perm ? "delete" : ",delete");
first_perm = false;
}
if (pub->pubactions.pubtruncate)
{
appendStringInfo(buf, first_perm ? "truncate" : ",truncate");
}

======
src/test/regress/expected/publication_ddl.out

18.
+ CREATE PUBLICATION testpub_ddl_1
+
+ WITH (publish='insert, update, delete, truncate',
publish_generated_columns='none', publish_via_partition_root='false');

~

This "pretty" output appears to have "+" garbage in it. What's that
about -- it looks like some sort of line continuation character? Can
it be removed?

~~~

19.
The "pretty" output might be improved if each of the publication
options was on a new line.

~~~

20.
The generated boolean values (e.g. 'true'/'false') do not need to be quoted.

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

(Here are lots of test review comments; the first group are are
general so might apply to multiple test cases).

21.
I think you can create all the necessary schema and tables together
up-front instead of scatering them through the file.

~~~

22.
Make use of the proper publication teminology like "Column Lists" and
"Row Filters" instead of vague
"columns" and "conditions".

~~~

23.
Here is an idea:

Instead of having dozens of test publications, just have 1 test
publication, which you CREATE/DROP for each test case.

Then, since there is a fixed name publication (e.g. "mypub") for
everything, you can make a subroutine to encapsulate the common code:

+SELECT pg_get_publication_ddl('mypub');
+SELECT pg_get_publication_ddl((SELECT oid FROM pg_publication WHERE
pubname='mypub'));
+SELECT pg_get_publication_ddl('mypub', 'pretty', 'true');

It means your test .sql file can become much shorter/simpler I think.

~~~

24.
There is duplication of some tests:

e.g.
+-- columns in publication must be quoted
and
+-- identifiers that require quoting: publication, schema, table and column

~~~

25.
It is not needed to quote the booleans 'true'/'false' for the options.

//////

26.
+-- create base table to test basic table publication

What does "basic table publication" mean? I expect it means different
things to different people. Better to be explicit about what this is
really testing.

~~~

27.
+-- create publication for one table with two columns and a condition
with an expression

What does "with an expression" mean? All Row-Filters are expressions
aren't they?

~~~

28.
+-- create a publication for a list of tables

Not really describing what this test is doing, which is mixing FOR
TABLE and FOR TABLES IN SCHEMA.

~~~

29.
+CREATE PUBLICATION testpub_ddl_part3 FOR TABLE testpub_ddl_part WITH
(publish_via_partition_root='true');

I guess it make no difference since these are only DDL syntax tests,
but it didn't really make much sense to set
publish_via_partition_root=true when testpub_ddl_part is the ROOT
anyway.

~~~

30.
+-- create publication for all tables except two tables

Actually this is also combining with an ALL SEQUENCES test.

~~~

31.
+-- cleanup publications
+DROP PUBLICATION testpub_ddl_1;
+DROP PUBLICATION testpub_ddl_2;
+DROP PUBLICATION testpub_ddl_3;
+DROP PUBLICATION testpub_ddl_4;
+DROP PUBLICATION testpub_ddl_5;
+DROP PUBLICATION testpub_ddl_6;
+DROP PUBLICATION testpub_ddl_7;
+DROP PUBLICATION testpub_ddl_8;
+DROP PUBLICATION testpub_ddl_9;
+DROP PUBLICATION testpub_ddl_10;
+DROP PUBLICATION testpub_ddl_schema_1;
+DROP PUBLICATION testpub_ddl_schema_2;
+DROP PUBLICATION testpub_ddl_schema_3;
+DROP PUBLICATION testpub_ddl_schema_4;
+DROP PUBLICATION testpub_ddl_schema_5;
+DROP PUBLICATION testpub_ddl_part1;
+DROP PUBLICATION testpub_ddl_part2;
+DROP PUBLICATION testpub_ddl_part3;
+DROP PUBLICATION testpub_ddl_part4;
+DROP PUBLICATION "Quoted Pub";
+DROP PUBLICATION testpub_ddl_except1;
+DROP PUBLICATION testpub_ddl_except2;

As per earlier review comment IMO it would make the test code simpler
to have just 1 publication that you CREATE/DROP om the fly.

~~~

32.
+-- cleanup tables in schemas

Not sure why this is done separately. Probably easier just to drop the
schemas with CASCADE so their tables will be auto-deleted.

======
Kind Regards,
Peter Smith.
Fujitsu Australia