On 14/09/16 21:53, Andres Freund wrote:
Hi,
On 2016-09-14 21:17:42 +0200, Petr Jelinek wrote:
+/*
+ * Gather Relations based o provided by RangeVar list.
+ * The gathered tables are locked in access share lock mode.
+ */
Why access share? Shouldn't we make this ShareUpdateExclusive or
similar, to prevent schema changes?
Hm, I thought AccessShare would be enough to prevent schema changes that
matter to us (which is basically just drop afaik).
Doesn't e.g. dropping an index matter as well?
Drop of primary key matters I guess.
+ if (strcmp($1, "replicate_insert") == 0)
+ $$ =
makeDefElem("replicate_insert",
+
(Node *)makeInteger(TRUE), @1);
+ else if (strcmp($1,
"noreplicate_insert") == 0)
+ $$ =
makeDefElem("replicate_insert",
+
(Node *)makeInteger(FALSE), @1);
+ else if (strcmp($1, "replicate_update")
== 0)
+ $$ =
makeDefElem("replicate_update",
+
(Node *)makeInteger(TRUE), @1);
+ else if (strcmp($1,
"noreplicate_update") == 0)
+ $$ =
makeDefElem("replicate_update",
+
(Node *)makeInteger(FALSE), @1);
+ else if (strcmp($1, "replicate_delete")
== 0)
+ $$ =
makeDefElem("replicate_delete",
+
(Node *)makeInteger(TRUE), @1);
+ else if (strcmp($1,
"noreplicate_delete") == 0)
+ $$ =
makeDefElem("replicate_delete",
+
(Node *)makeInteger(FALSE), @1);
+ else
+ ereport(ERROR,
+
(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("unrecognized
publication option \"%s\"", $1),
+
parser_errposition(@1)));
+ }
+ ;
I'm kind of inclined to do this checking at execution (or transform)
time instead. That allows extension to add options, and handle them in
utility hooks.
Thant's interesting point, I prefer the parsing to be done in gram.y, but it
might be worth moving it for extensibility. Although there are so far other
barriers for that.
Citus uses the lack of such check for COPY to implement copy over it's
distributed tables for example. So there's some benefit.
Yeah I am not saying that I am fundamentally against it, I am just
saying it won't help all that much probably.
+ check_subscription_permissions();
+
+ rel = heap_open(SubscriptionRelationId, RowExclusiveLock);
+
+ /* Check if name is used */
+ subid = GetSysCacheOid2(SUBSCRIPTIONNAME, MyDatabaseId,
+
CStringGetDatum(stmt->subname));
+ if (OidIsValid(subid))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("subscription \"%s\" already exists",
+ stmt->subname)));
+ }
+
+ /* Parse and check options. */
+ parse_subscription_options(stmt->options, &enabled_given, &enabled,
+ &conninfo,
&publications);
+
+ /* TODO: improve error messages here. */
+ if (conninfo == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("connection not specified")));
Probably also makes sense to parse the conninfo here to verify it looks
saen. Although that's fairly annoying to do, because the relevant code
is libpq :(
Well the connection is eventually used (in later patches) so maybe that's
not problem.
Well, it's nicer if it's immediately parsed, before doing complex and
expensive stuff, especially if that happens outside of the transaction.
Maybe, it's not too hard to add another function to libpqwalreceiver I
guess.
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 65230e2..f3d54c8 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
I think you might be missing outfuncs support.
I thought that we don't do outfuncs for DDL?
I think it's just readfuncs that's skipped.
I see only couple odd DDL commands in outfuncs.c.
+ Length of column name (including the NULL-termination
+ character).
+</para>
+</listitem>
+</varlistentry>
+<varlistentry>
+<term>
+ String
+</term>
+<listitem>
+<para>
+ Name of the column.
+</para>
+</listitem>
+</varlistentry>
Huh, no type information?
It's not necessary for the text transfer, it will be if we ever add binary
data transfer but that will require protocol version bump anyway.
I'm *hugely* unconvinced of this. For one type information is useful for
error reporting and such as well. For another, it's one thing to add a
new protocol message (for differently encoded tuples), and something
entirely different to change the format of existing messages.
Well it's one if on wrrite and one if on read side in this case, but I
can add it, it's rather simple change. One thing that we need to clarify
is how we actually send type info, I think for builtin types Oid should
be enough, but for all other ones we need qualified name of the type IMHO.
+
+/*
+ * COMMIT callback
+ */
+static void
+pgoutput_commit_txn(LogicalDecodingContext *ctx, ReorderBufferTXN *txn,
+ XLogRecPtr commit_lsn)
+{
+ OutputPluginPrepareWrite(ctx, true);
+ logicalrep_write_commit(ctx->out, txn, commit_lsn);
+ OutputPluginWrite(ctx, true);
+}
Hm, so we don't reset the context for these...
What?
We only use & reset the data-> memory context in the change
callback. I'm not sure that's good.
Well we don't do anything with the data memory context here.
This however I'm not following. Why do we need multiple copies of this?
And why aren't we doing the assignments in _PG_init? Seems better to
just allocate one WalRcvCalllbacks globally and assign all these as
constants. Then the establishment function can just return all these
(as part of a bigger struct).
Meh, If I understand you correctly that will make the access bit more ugly
(multiple layers of structs).
On the other hand, you right now need to access one struct, and pass the
other...
Point taken.
This does rather reinforce my opinion that the _PG_init removal in
libpqwalreceiver isn't useful.
I don't see how it helps, you said we'd still return struct from some
interface so this would be more or less the same?
Or we just set some global vars and use them directly.
I really hate the "global vars filled by external library when loaded"
as design pattern, it's how it was done before but it's ugly, especially
when you share the library between multiple C modules later.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
