Re: Proposal: Generic WAL logical messages

Hi,

On 2016-02-28 22:44:12 +0100, Petr Jelinek wrote:
> On 27/02/16 01:05, Andres Freund wrote:
> >I'm not really convinced by RegisterStandbyMsgPrefix() et al. There's
> >not much documentation about what it actually is supposed to
> >acomplish. Afaics you're basically forced to use
> >shared_preload_libraries with it right now? Also, iterating through a
> >linked list everytime something is logged doesn't seem very satisfying?
> >
>
> Well, my reasoning there was to stop multiple plugins from using same prefix
> and for that you need some kind of registry. Making this a shared catalog
> seemed like huge overkill given the potentially transient nature of output
> plugins and this was the best I could come up with. And yes it requires you
> to load your plugin before you can log a message for it.

I think right now that's a solution that's worse than the problem. I'm
inclined to define the problem away with something like "The prefix
should be unique across different users of the messaging facility. Using
the extension name often is a good choice.".

> >>+void
> >>+logicalmsg_desc(StringInfo buf, XLogReaderState *record)
> >>+{
> >>+ char *rec = XLogRecGetData(record);
> >>+ xl_logical_message *xlrec = (xl_logical_message *) rec;
> >>+
> >>+ appendStringInfo(buf, "%s message size %zu bytes",
> >>+ xlrec->transactional ? "transactional" : "nontransactional",
> >>+ xlrec->message_size);
> >>+}
> >
> >Shouldn't we check
> > uint8 info = XLogRecGetInfo(record) & ~XLR_INFO_MASK;
> > if XLogRecGetInfo(record) == XLOG_LOGICAL_MESSAGE
> >here?
> >
>
> I thought it's kinda pointless, but we seem to be doing it in other places
> so will add.

It leads to a segfault or something similar when adding further message
types, without a compiler warning. So it seems to be a good idea to be
slightly careful.

> >
> >>+void
> >>+ReorderBufferQueueMessage(ReorderBuffer *rb, TransactionId xid, XLogRecPtr lsn,
> >>+ bool transactional, const char *prefix, Size msg_sz,
> >>+ const char *msg)
> >>+{
> >>+ ReorderBufferTXN *txn = NULL;
> >>+
> >>+ if (transactional)
> >>+ {
> >>+ ReorderBufferChange *change;
> >>+
> >>+ txn = ReorderBufferTXNByXid(rb, xid, true, NULL, lsn, true);
> >>+
> >>+ Assert(xid != InvalidTransactionId);
> >>+ Assert(txn != NULL);
> >>+
> >>+ change = ReorderBufferGetChange(rb);
> >>+ change->action = REORDER_BUFFER_CHANGE_MESSAGE;
> >>+ change->data.msg.transactional = true;
> >>+ change->data.msg.prefix = pstrdup(prefix);
> >>+ change->data.msg.message_size = msg_sz;
> >>+ change->data.msg.message = palloc(msg_sz);
> >>+ memcpy(change->data.msg.message, msg, msg_sz);
> >>+
> >>+ ReorderBufferQueueChange(rb, xid, lsn, change);
> >>+ }
> >>+ else
> >>+ {
> >>+ rb->message(rb, txn, lsn, transactional, prefix, msg_sz, msg);
> >>+ }
> >>+}
> >
> >
> >This approach prohibts catalog access when processing a nontransaction
> >message as there's no snapshot set up.
> >
>
> Hmm I do see usefulness in having snapshot, although I wonder if that does
> not kill the point of non-transactional messages.

I don't see how it would? It'd obviously have to be the catalog/historic
snapshot a transaction would have had if it started in that moment in
the original WAL stream?

> Question is then though which snapshot should the message see,
> base_snapshot of transaction?

Well, there'll not be a transaction, but something like snapbuild.c's
->snapshot ought to do the trick.

> That would mean we'd have to call SnapBuildProcessChange for
> non-transactional messages which we currently avoid. Alternatively we
> could probably invent lighter version of that interface that would
> just make sure builder->snapshot is valid and if not then build it

I think the latter is probably the direction we should go in.

> I am honestly sure if that's a win or not.

I think it'll be confusing (bug inducing) if there's no snapshot for
non-transactional messages but for transactional ones, and it'll
severely limit the usefulness of the interface.

Andres