Proposal: Generic WAL logical messages

Petr Jelinek wrote:

These messages just create WAL record with arbitrary data as specified by
user. For standard WAL reply, these are basically noop, but in logical
decoding they are be decoded and the special callback of the output plugin
is be called for them.

I had a quick look at this and I couldn't find anything worthy of
comment -- seems a reasonably straightforward addition to stuff that's
mostly already there.

Process-wise, I don't understand why is Andres mentioned as co-author.
Did he actually wrote part of the patch, or advised on the design?
Who is reviewing the patch?

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Process-wise, I don't understand why is Andres mentioned as co-author.
Did he actually wrote part of the patch, or advised on the design?
Who is reviewing the patch?

It's extracted & extended from BDR, where I added that feature (to
implement distributed locking).

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 1 January 2016 at 03:59, Petr Jelinek <petr@2ndquadrant.com> wrote:

I would like to introduce concept of generic WAL logical messages.

Couple of points...

* Genenric misspelled

* You call them "logical messages" here, but standby messages in code. But
they only apply to logical decoding, so "logical message" seems a better
name. Can we avoid calling them "messages" cos that will get confusing.

For standard WAL reply, these are basically noop

We should document that.

These messages can be both transactional (decoded on commit) or
non-transactional (decoded immediately). Each message has prefix to
differentiate between individual plugins. The prefix has to be registered
exactly once (in similar manner as security label providers) to avoid
conflicts between plugins.

I'm not sure what "transactional" means, nor is that documented.
(Conversely, I think "immediate" fairly clear)

Are they fired only on commit? (Guess so)
Are they fired in the original order, if multiple messages in same
transaction? (Hope so)
Are they fired as they come in the original message sequence, or before
anything else or after everything else? For example, cache invalidation
messages are normally fired right at the end of a transaction, no matter
when they were triggered.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2016-01-07 17:22, Simon Riggs wrote:

* You call them "logical messages" here, but standby messages in code.
But they only apply to logical decoding, so "logical message" seems a
better name. Can we avoid calling them "messages" cos that will get
confusing.

Yes it's slightly confusing, the "Standby" in the code is mostly for
consistency with other "Standby*" stuff in neighbouring code, but that
can be changed. I don't have better name than "messages" though,
"records" is too generic.

For standard WAL reply, these are basically noop

We should document that.

Okay.

These messages can be both transactional (decoded on commit) or
non-transactional (decoded immediately). Each message has prefix to
differentiate between individual plugins. The prefix has to be
registered exactly once (in similar manner as security label
providers) to avoid conflicts between plugins.

I'm not sure what "transactional" means, nor is that documented.
(Conversely, I think "immediate" fairly clear)

Are they fired only on commit? (Guess so)
Are they fired in the original order, if multiple messages in same
transaction? (Hope so)
Are they fired as they come in the original message sequence, or before
anything else or after everything else? For example, cache invalidation
messages are normally fired right at the end of a transaction, no matter
when they were triggered.

Transnational message is added to the stream same way as any DML
operation is and has same properties (processed on commit, original
order, duplicate messages are delivered as they are).

The immediate as you say is obvious, they get to logical decoding
immediately without dealing with any regards to what's happening around
(wal order is still preserved though).

Will make this clearer in the docs.

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

Hi,

here is updated version of this patch, calling the messages logical
(decoding) messages consistently everywhere and removing any connection
to standby messages. Moving this to it's own module gave me place to
write some brief explanation about this so the code documentation has
hopefully improved as well.

The functionality itself didn't change.

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

Hi, Petr!

On Sat, Jan 23, 2016 at 1:22 AM, Petr Jelinek <petr@2ndquadrant.com> wrote:

here is updated version of this patch, calling the messages logical
(decoding) messages consistently everywhere and removing any connection to
standby messages. Moving this to it's own module gave me place to write
some brief explanation about this so the code documentation has hopefully
improved as well.

The functionality itself didn't change.

I'd like to mention that there is my upcoming patch which is named generic
WAL records.
*/messages/by-id/CAPpHfdsXwZmojm6Dx+TJnpYk27kT4o7Ri6X_4OSWcByu1Rm+VA@mail.gmail.com
</messages/by-id/CAPpHfdsXwZmojm6Dx+TJnpYk27kT4o7Ri6X_4OSWcByu1Rm+VA@mail.gmail.com&gt;*
But it has to be distinct feature from your generic WAL logical messages.
Theoretically, we could have generic messages with arbitrary content and
both having custom WAL reply function and being decoded by output plugin.
But custom WAL reply function would let extension bug break recovery,
archiving and physical replication. And that doesn't seem to be acceptable.
This is why we have to develop these as separate features.

Should we think more about naming? Does two kinds of generic records
confuse people?

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 29 January 2016 at 21:11, Alexander Korotkov <a.korotkov@postgrespro.ru>
wrote:

Hi, Petr!

On Sat, Jan 23, 2016 at 1:22 AM, Petr Jelinek <petr@2ndquadrant.com>
wrote:

here is updated version of this patch, calling the messages logical
(decoding) messages consistently everywhere and removing any connection to
standby messages. Moving this to it's own module gave me place to write
some brief explanation about this so the code documentation has hopefully
improved as well.

The functionality itself didn't change.

I'd like to mention that there is my upcoming patch which is named generic
WAL records.
*/messages/by-id/CAPpHfdsXwZmojm6Dx+TJnpYk27kT4o7Ri6X_4OSWcByu1Rm+VA@mail.gmail.com
</messages/by-id/CAPpHfdsXwZmojm6Dx+TJnpYk27kT4o7Ri6X_4OSWcByu1Rm+VA@mail.gmail.com&gt;*
But it has to be distinct feature from your generic WAL logical messages.
Theoretically, we could have generic messages with arbitrary content and
both having custom WAL reply function and being decoded by output plugin.
But custom WAL reply function would let extension bug break recovery,
archiving and physical replication. And that doesn't seem to be acceptable.
This is why we have to develop these as separate features.

Should we think more about naming? Does two kinds of generic records
confuse people?

Logical messages

Generic WAL records

Seems like I can tell them apart. Worth checking, but I think we're OK.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sat, Jan 30, 2016 at 11:58 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 29 January 2016 at 21:11, Alexander Korotkov <a.korotkov@postgrespro.ru

wrote:

Hi, Petr!

On Sat, Jan 23, 2016 at 1:22 AM, Petr Jelinek <petr@2ndquadrant.com>
wrote:

here is updated version of this patch, calling the messages logical
(decoding) messages consistently everywhere and removing any connection to
standby messages. Moving this to it's own module gave me place to write
some brief explanation about this so the code documentation has hopefully
improved as well.

The functionality itself didn't change.

I'd like to mention that there is my upcoming patch which is named
generic WAL records.
*/messages/by-id/CAPpHfdsXwZmojm6Dx+TJnpYk27kT4o7Ri6X_4OSWcByu1Rm+VA@mail.gmail.com
</messages/by-id/CAPpHfdsXwZmojm6Dx+TJnpYk27kT4o7Ri6X_4OSWcByu1Rm+VA@mail.gmail.com&gt;*
But it has to be distinct feature from your generic WAL logical messages.
Theoretically, we could have generic messages with arbitrary content and
both having custom WAL reply function and being decoded by output plugin.
But custom WAL reply function would let extension bug break recovery,
archiving and physical replication. And that doesn't seem to be acceptable.
This is why we have to develop these as separate features.

Should we think more about naming? Does two kinds of generic records
confuse people?

Logical messages

Generic WAL records

Seems like I can tell them apart. Worth checking, but I think we're OK.

I was worrying because topic name is "Generic WAL logical messages". But if
we name them just "Logical messages" then it's OK for me.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 1 February 2016 at 09:45, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:

On Sat, Jan 30, 2016 at 11:58 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 29 January 2016 at 21:11, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:

Should we think more about naming? Does two kinds of generic records
confuse people?

Logical messages

Generic WAL records

Seems like I can tell them apart. Worth checking, but I think we're OK.

I was worrying because topic name is "Generic WAL logical messages". But if
we name them just "Logical messages" then it's OK for me.

Yeah the patch talks about logical messages, I use different title in
mailing list and CF to make it more clear on first sight what this
actually is technically.

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

On 23.01.2016 01:22, Petr Jelinek wrote:

Hi,

here is updated version of this patch, calling the messages logical
(decoding) messages consistently everywhere and removing any connection
to standby messages. Moving this to it's own module gave me place to
write some brief explanation about this so the code documentation has
hopefully improved as well.

The functionality itself didn't change.

Hello,

It seems that you forgot regression tests for test_decoding. There is an
entry in test_decoding/Makefile, but there are not files
sql/messages.sql and expected/messages.out. However they are included in
the first version of the patch.

--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello,

It seems that you forgot regression tests for test_decoding. There is an
entry in test_decoding/Makefile, but there are not files
sql/messages.sql and expected/messages.out. However they are included in
the first version of the patch.

Hi, yes, git add missing.

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

Hi,

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?

On 2016-02-24 18:35:16 +0100, Petr Jelinek wrote:

+SET synchronous_commit = on;
+SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding');
+ ?column? 
+----------
+ init
+(1 row)

+SELECT 'msg1' FROM pg_logical_send_message(true, 'test', 'msg1');
+ ?column? 
+----------
+ msg1
+(1 row)

Hm. Somehow 'sending' a message seems wrong here. Maybe 'emit'?

+      <row>
+       <entry id="pg-logical-send-message-text">
+        <indexterm>
+         <primary>pg_logical_send_message</primary>
+        </indexterm>
+        <literal><function>pg_logical_send_message(<parameter>transactional</parameter> <type>bool</type>, <parameter>prefix</parameter> <type>text</type>, <parameter>content</parameter> <type>text</type>)</function></literal>
+       </entry>
+       <entry>
+        void
+       </entry>
+       <entry>
+        Write text logical decoding message. This can be used to pass generic
+        messages to logical decoding plugins through WAL. The parameter
+        <parameter>transactional</parameter> specifies if the message should
+        be part of current transaction or if it should be written and decoded
+        immediately. The <parameter>prefix</parameter> has to be prefix which
+        was registered by a plugin. The <parameter>content</parameter> is
+        content of the message.
+       </entry>
+      </row>

It's not decoded immediately, even if emitted non-transactionally.

+    <sect3 id="logicaldecoding-output-plugin-message">
+     <title>Generic Message Callback</title>
+
+     <para>
+      The optional <function>message_cb</function> callback is called whenever
+      a logical decoding message has been decoded.
+<programlisting>
+typedef void (*LogicalDecodeMessageCB) (
+    struct LogicalDecodingContext *,
+    ReorderBufferTXN *txn,
+    XLogRecPtr message_lsn,
+    bool transactional,
+    const char *prefix,
+    Size message_size,
+    const char *message
+);

We should at least document what txn is set to if not transactional.

+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?

+const char *
+logicalmsg_identify(uint8 info)
+{
+	return NULL;
+}

Huh?

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

+		case REORDER_BUFFER_CHANGE_MESSAGE:
+			{
+				char	   *data;
+				size_t		prefix_size = strlen(change->data.msg.prefix) + 1;
+
+				sz += prefix_size + change->data.msg.message_size;
+				ReorderBufferSerializeReserve(rb, sz);
+
+				data = ((char *) rb->outbuf) + sizeof(ReorderBufferDiskChange);
+				memcpy(data, change->data.msg.prefix,
+					   prefix_size);
+				memcpy(data + prefix_size, change->data.msg.message,
+					   change->data.msg.message_size);
+				break;
+			}
case REORDER_BUFFER_CHANGE_INTERNAL_SNAPSHOT:
{
Snapshot	snap;
@@ -2354,6 +2415,18 @@ ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
data += len;
}
break;
+		case REORDER_BUFFER_CHANGE_MESSAGE:
+			{
+				Size		message_size = change->data.msg.message_size;
+				Size		prefix_size = strlen(data) + 1;
+
+				change->data.msg.prefix = pstrdup(data);
+				change->data.msg.message = palloc(message_size);
+				memcpy(change->data.msg.message, data + prefix_size,
+					   message_size);
+
+				data += prefix_size + message_size;
+			}

Please add a test exercising these paths.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello,

On 27.02.2016 03:05, Andres Freund wrote:

Hi,

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?

I have did some tests with a simple plugin. I have used event triggers
to send messages. It works, but I agree with Andres. We have problems if
plugin is not loaded. For example, if you will execute the query:

SELECT 'msg2' FROM pg_logical_send_message(false, 'test', 'msg2');

you will get the error (if plugin which should register a prefix is not
loaded yet):

ERROR: standby message prefix "test" is not registered

Some stylistic note (logical.c):

+static void message_cb_wrapper(ReorderBuffer *cache, ReorderBufferTXN *txn,
+							   XLogRecPtr message_lsn,
+							   bool transactional, const char *prefix,
+							   Size sz, const char *message)
+{
+	LogicalDecodingContext *ctx = cache->private_data;
+	LogicalErrorCallbackState state;
+	ErrorContextCallback errcallback;

It should be written in the following way:

static void
message_cb_wrapper(ReorderBuffer *cache, ReorderBufferTXN *txn,
XLogRecPtr message_lsn,
bool transactional, const char *prefix,
Size sz, const char *message)
{
LogicalDecodingContext *ctx = cache->private_data;
LogicalErrorCallbackState state;
ErrorContextCallback errcallback;

--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

thanks for looking Andres,

On 27/02/16 01:05, Andres Freund wrote:

Hi,

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 am not married to this solution so if you have better ideas or if you
think the clashes are not read issue, we can change it.

On 2016-02-24 18:35:16 +0100, Petr Jelinek wrote:

+SET synchronous_commit = on;
+SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding');
+ ?column?
+----------
+ init
+(1 row)

+SELECT 'msg1' FROM pg_logical_send_message(true, 'test', 'msg1');
+ ?column?
+----------
+ msg1
+(1 row)

Hm. Somehow 'sending' a message seems wrong here. Maybe 'emit'?

+      <row>
+       <entry id="pg-logical-send-message-text">
+        <indexterm>
+         <primary>pg_logical_send_message</primary>
+        </indexterm>
+        <literal><function>pg_logical_send_message(<parameter>transactional</parameter> <type>bool</type>, <parameter>prefix</parameter> <type>text</type>, <parameter>content</parameter> <type>text</type>)</function></literal>
+       </entry>
+       <entry>
+        void
+       </entry>
+       <entry>
+        Write text logical decoding message. This can be used to pass generic
+        messages to logical decoding plugins through WAL. The parameter
+        <parameter>transactional</parameter> specifies if the message should
+        be part of current transaction or if it should be written and decoded
+        immediately. The <parameter>prefix</parameter> has to be prefix which
+        was registered by a plugin. The <parameter>content</parameter> is
+        content of the message.
+       </entry>
+      </row>

It's not decoded immediately, even if emitted non-transactionally.

Okay, immediately is somewhat misleading. How does "should be written
immediately and decoded as soon as logical decoding reads the WAL
record" sound ?

+    <sect3 id="logicaldecoding-output-plugin-message">
+     <title>Generic Message Callback</title>
+
+     <para>
+      The optional <function>message_cb</function> callback is called whenever
+      a logical decoding message has been decoded.
+<programlisting>
+typedef void (*LogicalDecodeMessageCB) (
+    struct LogicalDecodingContext *,
+    ReorderBufferTXN *txn,
+    XLogRecPtr message_lsn,
+    bool transactional,
+    const char *prefix,
+    Size message_size,
+    const char *message
+);

We should at least document what txn is set to if not transactional.

Will do (it's NULL).

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

+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. Question is then
though which snapshot should the message see, base_snapshot of
transaction? 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 but
I am honestly sure if that's a win or not.

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

I think here

+const char *
+logicalmsg_identify(uint8 info)
+{
+	if (info & ~XLR_INFO_MASK == XLOG_LOGICAL_MESSAGE)
+		return "MESSAGE";
+
+	return NULL;
+}

we should use brackets

const char *
logicalmsg_identify(uint8 info)
{
if ((info & ~XLR_INFO_MASK) == XLOG_LOGICAL_MESSAGE)
return "MESSAGE";

return NULL;
}

Because of operator priorities
http://en.cppreference.com/w/c/language/operator_precedence we may get
errors.

On 01.03.2016 00:10, Petr Jelinek wrote:

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)

--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 08/03/16 21:21, Artur Zakirov wrote:

I think here

+const char *
+logicalmsg_identify(uint8 info)
+{
+    if (info & ~XLR_INFO_MASK == XLOG_LOGICAL_MESSAGE)
+        return "MESSAGE";
+
+    return NULL;
+}

we should use brackets

const char *
logicalmsg_identify(uint8 info)
{
if ((info & ~XLR_INFO_MASK) == XLOG_LOGICAL_MESSAGE)
return "MESSAGE";

return NULL;
}

Correct, fixed, thanks.

I also rebased this as there was conflict after the fixes to logical
decoding by Andres.

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

On 3/9/16 6:58 PM, Petr Jelinek wrote:

On 08/03/16 21:21, Artur Zakirov wrote:

I think here

+const char *
+logicalmsg_identify(uint8 info)
+{
+    if (info & ~XLR_INFO_MASK == XLOG_LOGICAL_MESSAGE)
+        return "MESSAGE";
+
+    return NULL;
+}

we should use brackets

const char *
logicalmsg_identify(uint8 info)
{
if ((info & ~XLR_INFO_MASK) == XLOG_LOGICAL_MESSAGE)
return "MESSAGE";

return NULL;
}

Correct, fixed, thanks.

I also rebased this as there was conflict after the fixes to logical
decoding by Andres.

This patch applies cleanly and is ready for review with no outstanding
issues that I can see. Simon and Artur, you are both signed up as
reviewers. Care to take a crack at it?

Thanks,
--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers