Re: Logical Replication WIP

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