Hi Vignesh.
Some review comments for the patch v45-0004.
======
src/bin/psql/describe.c
describeSubscriptions:
1.
+ if (pset.sversion >= 190000)
+ {
+ appendPQExpBuffer(&buf,
+ ", (select srvname from pg_foreign_server where oid=subserver) AS \"%s\"\n",
+ gettext_noop("Server"));
- /* Skip LSN is only supported in v15 and higher */
- if (pset.sversion >= 150000)
- appendPQExpBuffer(&buf,
- ", subskiplsn AS \"%s\"\n",
- gettext_noop("Skip LSN"));
+ appendPQExpBuffer(&buf,
+ ", subretaindeadtuples AS \"%s\"\n",
+ gettext_noop("Retain dead tuples"));
appendPQExpBuffer(&buf,
- ", pg_catalog.obj_description(oid, 'pg_subscription') AS \"%s\"\n",
- gettext_noop("Description"));
+ ", submaxretention AS \"%s\"\n",
+ gettext_noop("Max retention duration"));
+
+ appendPQExpBuffer(&buf,
+ ", subretentionactive AS \"%s\"\n",
+ gettext_noop("Retention active"));
+
+ ncols += 4;
+ }
These can all be combined together, so there is just a single
appendPQExpBuffer. Same as other single code fragments in the same
function.
~~~
2.
+ appendPQExpBuffer(&buf,
+ ", pg_catalog.obj_description(oid, 'pg_subscription') AS \"%s\"\n",
+ gettext_noop("Description"));
+ ncols++;
+
+ /* Conflict log destination is supported in v19 and higher */
+ if (pset.sversion >= 190000)
+ {
+ appendPQExpBuffer(&buf,
+ ", subconflictlogdest AS \"%s\"\n",
+ gettext_noop("Conflict log destination"));
+ ncols++;
I think the "Description" should remain as the very last column of the
table, same as it always has been. All those others are options so
they belong together, but description is different.
~~~
3.
+ printTableAddHeader(&cont, gettext_noop("Description"), true, align);
+ printTableAddCell(&cont, PQgetvalue(res, i, current_col++), false, false);
+
+ if (pset.sversion >= 190000)
+ {
+ char *logdest;
+
+ printTableAddHeader(&cont, gettext_noop("Conflict log destination"),
+ true, align);
Ditto. Leave the description as the very last column.
~~~
4.
+ if (pset.sversion >= 190000)
+ {
+ char *logdest;
+
+ printTableAddHeader(&cont, gettext_noop("Conflict log destination"),
+ true, align);
+
+ logdest = PQgetvalue(res, i, current_col++);
+
+ printTableAddCell(&cont, logdest, false, false);
+
+ if (strcmp(logdest, "table") == 0 ||
+ strcmp(logdest, "all") == 0)
+ {
+ char conflictlogtable[NAMEDATALEN + 32];
+
+ snprintf(conflictlogtable,
+ sizeof(conflictlogtable),
+ "%s.pg_conflict_log_for_subid_%s",
+ conflict_schema, subid);
+
+ printTableAddFooter(&cont, _("Conflict log table:"));
+ printTableAddFooter(&cont, psprintf(" %s", conflictlogtable));
+ }
+ }
+
4a.
This 'snprintf' is yet another code fragment that is building the
generated CLT name. This should be using some common function to do
it. See my review comment for an earlier patch in this set [1, comment
#2].
~~~
4b.
+ printTableAddFooter(&cont, _("Conflict log table:"));
+ printTableAddFooter(&cont, psprintf(" %s", conflictlogtable));
Normally in these describe commands, the footer part (e.g. "Except
tables:" etc) puts the names in quotes. So, even though it may not be
strictly necessary, this should do the same for consistency:
e.g.
BEFORE
Conflict log table:
pg_conflict.pg_conflict_log_for_subid_16392
AFTER
Conflict log table:
"pg_conflict.pg_conflict_log_for_subid_16392"
~~~
4c.
IMO there should be a separate function for handling the subscription
footer/s, same as there is already a function
addFooterToPublicationDesc.
======
src/test/regress/expected/subscription.out
Some general comments about the describe testing:
5.
AFAICT there this patch is not being tested properly because there are
no tests where the conflict_log_destination is "table" or "all".
~~~
6.
Even though the \dRs code was changed a lot, it seems there were no
impacted tests because they were all using \dRs+ and never \dRs. So I
think there needs to be more "describe" tests that are using *both*
forms of this command.
Also consider using expanded form \x for some of the \dRs and \dRs+
tests, so that the expected output is easier to read.
======
[1]
https://www.postgresql.org/message-id/CAHut%2BPt3%3DrZO%2ByJqj7o3xH0LcgrztFCvhoayiBJWbhmEio6teQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
