Re: Logical Replication WIP - Mailing list pgsql-hackers

Hi,

(btw, I vote against tarballing patches)

+   <tgroup cols="4">
+    <thead>
+     <row>
+      <entry>Name</entry>
+      <entry>Type</entry>
+      <entry>References</entry>
+      <entry>Description</entry>
+     </row>
+    </thead>
+
+    <tbody>
+     <row>
+      <entry><structfield>oid</structfield></entry>
+      <entry><type>oid</type></entry>
+      <entry></entry>
+      <entry>Row identifier (hidden attribute; must be explicitly selected)</entry>
+     </row>
+

+     <row>
+      <entry><structfield>subpublications</structfield></entry>
+      <entry><type>name[]</type></entry>
+      <entry></entry>
+      <entry>Array of subscribed publication names. These reference the
+       publications on the publisher server.
+      </entry>

Why is this names and not oids? So you can see it across databases?

I think this again should have an owner.

include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 68d7e46..523008d 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -112,6 +112,7 @@ static event_trigger_support_data event_trigger_support[] = {    {"SCHEMA", true},    {"SEQUENCE",
true},   {"SERVER", true},
 
+    {"SUBSCRIPTION", true},

Hm, is that ok? Subscriptions are shared, so ...?


+        /*
+         * If requested, create the replication slot on remote side for our
+         * newly created subscription.
+         *
+         * Note, we can't cleanup slot in case of failure as reason for
+         * failure might be already existing slot of the same name and we
+         * don't want to drop somebody else's slot by mistake.
+         */
+        if (create_slot)
+        {
+            XLogRecPtr            lsn;
+
+            /*
+             * Create the replication slot on remote side for our newly created
+             * subscription.
+             *
+             * Note, we can't cleanup slot in case of failure as reason for
+             * failure might be already existing slot of the same name and we
+             * don't want to drop somebody else's slot by mistake.
+             */

We should really be able to recognize that based on the error code...

+/*
+ * Drop subscription by OID
+ */
+void
+DropSubscriptionById(Oid subid)
+{

+    /*
+     * We must ignore errors here as that would make it impossible to drop
+     * subscription when publisher is down.
+     */

I'm not convinced.  Leaving a slot around without a "record" of it on
the creating side isn't nice either. Maybe a FORCE flag or something?

+subscription_create_opt_item:
+            subscription_opt_item
+            | INITIALLY IDENT
+                {
+                    if (strcmp($2, "enabled") == 0)
+                        $$ = makeDefElem("enabled",
+                                         (Node *)makeInteger(TRUE), @1);
+                    else if (strcmp($2, "disabled") == 0)
+                        $$ = makeDefElem("enabled",
+                                         (Node *)makeInteger(FALSE), @1);
+                    else
+                        ereport(ERROR,
+                                (errcode(ERRCODE_SYNTAX_ERROR),
+                                 errmsg("unrecognized subscription option \"%s\"", $1),
+                                     parser_errposition(@2)));
+                }
+            | IDENT
+                {
+                    if (strcmp($1, "create_slot") == 0)
+                        $$ = makeDefElem("create_slot",
+                                         (Node *)makeInteger(TRUE), @1);
+                    else if (strcmp($1, "nocreate_slot") == 0)
+                        $$ = makeDefElem("create_slot",
+                                         (Node *)makeInteger(FALSE), @1);
+                }
+        ;

Hm, the IDENT case ignores $1 if it's not create_slot/nocreate_slot and
thus leaves $$ uninitialized?

I again really would like to have the error checking elsewhere.



- Andres