Re: Handle infinite recursion in logical replication setup

On Thu, Jun 30, 2022 at 9:40 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, Jun 30, 2022 at 9:17 AM shiy.fnst@fujitsu.com
> <shiy.fnst@fujitsu.com> wrote:
> >

The first patch that adds a test case for existing functionality looks
good to me and I'll push that early next week (by Tuesday) unless
there are more comments on it.

Few minor comments on 0002
========================
1.
+ /* FIXME: 150000 should be changed to 160000 later for PG16. */
+ if (options->proto.logical.origin &&
+ PQserverVersion(conn->streamConn) >= 150000)
+ appendStringInfo(&cmd, ", origin '%s'",
+ options->proto.logical.origin);

...
...
+ /* FIXME: 150000 should be changed to 160000 later for PG16. */
+ if (fout->remoteVersion >= 150000)
+ appendPQExpBufferStr(query, " s.suborigin\n");
+ else
+ appendPQExpBuffer(query, " '%s' AS suborigin\n", LOGICALREP_ORIGIN_ANY);
...
...
+ /* FIXME: 150000 should be changed to 160000 later for PG16 */
+ if (pset.sversion >= 150000)
+ appendPQExpBuffer(&buf,
+   ", suborigin AS \"%s\"\n",
+   gettext_noop("Origin"));

All these should now change to 16.

2.
/* ALTER SUBSCRIPTION <name> SET ( */
  else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
TailMatches("SET", "("))
- COMPLETE_WITH("binary", "slot_name", "streaming",
"synchronous_commit", "disable_on_error");
+ COMPLETE_WITH("binary", "origin", "slot_name", "streaming",
"synchronous_commit", "disable_on_error");
  /* ALTER SUBSCRIPTION <name> SKIP ( */
  else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
TailMatches("SKIP", "("))
  COMPLETE_WITH("lsn");
@@ -3152,7 +3152,7 @@ psql_completion(const char *text, int start, int end)
  /* Complete "CREATE SUBSCRIPTION <name> ...  WITH ( <opt>" */
  else if (HeadMatches("CREATE", "SUBSCRIPTION") && TailMatches("WITH", "("))
  COMPLETE_WITH("binary", "connect", "copy_data", "create_slot",
-   "enabled", "slot_name", "streaming",
+   "enabled", "origin", "slot_name", "streaming",
    "synchronous_commit", "two_phase", "disable_on_error");

Why do you choose to add a new option in-between other parameters
instead of at the end which we normally do? The one possible reason I
can think of is that all the parameters at the end are boolean so you
want to add this before those but then why before slot_name, and again
I don't see such a rule being followed for other parameters.

-- 
With Regards,
Amit Kapila.