Re: Proposal: Generic WAL logical messages

Hi,

attached is the newest version of the patch.

I removed the registry, renamed the 'send' to 'emit', documented the
callback parameters properly. I also added the test to ddl.sql for the
serialization and deserialization (and of course found a bug there) and
in general fixed all the stuff Andres reported.

(see more inline)

On 28/02/16 22:55, Andres Freund wrote:
>
>
>>>
>>>> +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.
>

Ok I added interface which returns either existing snapshot or makes new
one, seems like the most reasonable thing to do to me.

>
>> 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.
>

Nono, I meant I am not sure if special interface is a win over just
using SnapBuildProcessChange() in practice.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services