On 2/21/25 07:17, Masahiko Sawada wrote:
On Thu, Feb 20, 2025 at 12:50 PM Markus Wanner
wrote:
Hi,
I recently stumbled over an issue with an unintentional re-transmission.
While this clearly was our fault in the output plugin code, I think the
walsender's exposed API could easily be har
ached one line patch?
Best Regards
MarkusFrom 65dd24ed975cc513e4e0de5e175dff16b4b0f6d4 Mon Sep 17 00:00:00 2001
From: Markus Wanner
Date: Thu, 20 Feb 2025 21:01:45 +0100
Subject: [PATCH] Reset the output buffer after sending from WalSndWriteData
While not strictly necessary, clearing the buffer
On 6/24/22 00:54, Tom Lane wrote:
Does such code exist? I don't see any other calls in Debian code search,
and I find it hard to believe that anyone would think such a thing is
maintainable.
Such a thing does exist within PGLogical and BDR, yes.
Thanks for your concern about maintainability.
On 6/23/22 15:34, Tom Lane wrote:
Under what circumstances would it be OK for outside code to call
SPICleanup?
For the same reasons previous Postgres versions called SPICleanup: from
a sigsetjmp handler that duplicates most of what Postgres does in such a
situation.
However, I think that's
On 6/21/22 18:33, Tom Lane wrote:
My inclination at this point is to not back-patch the second change
12d768e70 ("Don't use static storage for SaveTransactionCharacteristics").
It's not clear that the benefit would be worth even a small risk of
somebody being unhappy about the API break.
Actua
On Mon, 2022-04-11 at 15:21 -0400, Robert Haas wrote:
> ... before v13, the commit in question actually
> changed the size of PGXACT, which is really quite bad -- it needs to
> be 12 bytes for performance reasons. And there's no spare bytes
> available, so I think we should follow one of the sugges
Chkpt from type bool to type int. So, change it back,
and add a new bool delayChkptEnd field instead. The new field should
fall within what used to be padding space within the struct, and so
hopefully won't cause any extensions to break.
Per report from Markus Wanner and discussion with
developers expect? Which parts are okay to change even in
stable branches and which can be relied upon to remain stable?
And for this specific case: Is it worth reverting this change and
applying a fully backwards compatible fix, instead?
Regards
Markus Wanner
: Our BDR extension indeed references the wait
events in question (or at least it used to do so up until that commit).
--
Markus Wanner
EDB: http://www.enterprisedb.com
On 31.03.21 15:18, Amit Kapila wrote:
On Wed, Mar 31, 2021 at 11:55 AM Markus Wanner
The last sentences there now seems to relate to just the setting of
"concurrent_abort", rather than the whole reason to invoke the
prepare_cb. And the reference to the "gid" is a bit lost.
On 31.03.21 06:39, Amit Kapila wrote:
I have slightly adjusted the comments, docs, and commit message. What
do you think about the attached?
Thank you both, Amit and Ajin. This looks good to me.
Only one minor gripe:
+ a prepared transaction with incomplete changes, in which case the
+
On 30.03.21 11:54, Markus Wanner wrote:
I would recommend this more explicit API and communication over hiding
the concurrent abort in a prepare callback.
I figured we already have the ReorderBufferTXN's concurrent_abort flag,
thus I agree the prepare_cb is sufficient and revoke
On 30.03.21 11:12, Ajin Cherian wrote:
I found some documentation that already was talking about concurrent
aborts and updated that.
Thanks.
I just noticed as of PG13, concurrent_abort is part of ReorderBufferTXN,
so it seems the prepare_cb (or stream_prepare_cb) can actually figure a
concur
On 30.03.21 11:02, Amit Kapila wrote:
On Tue, Mar 30, 2021 at 12:00 PM Markus Wanner
Yes, this replaces the PREPARE I would do from the concurrent_abort
callback in a direct call to rb->prepare.
Because concurrent_abort()
internally trying to prepare transaction seems a bit ugly and not o
On 30.03.21 10:33, Amit Kapila wrote:
Pushed. In the last version, you have named the patch incorrectly.
Thanks a lot, Amit!
Regards
Markus
On 30.03.21 09:39, Ajin Cherian wrote:
Where do you suggest this be documented? From an externally visible
point of view, I dont see much of a surprise.
If you start to think about the option of committing a prepared
transaction from a different node, the danger becomes immediately
apparent:
Hello Ajin,
On 30.03.21 06:48, Ajin Cherian wrote:
For now, I've created a patch that addresses the problem reported using
the existing callbacks.
Thanks.
Do have a look if this fixes the problem reported.
Yes, this replaces the PREPARE I would do from the concurrent_abort
callback in a d
On 29.03.21 14:00, vignesh C wrote:
Have you intentionally not
written any tests as it will be difficult to predict the xid. I just
wanted to confirm my understanding.
Yeah, that's the reason this is hard to test this with a regression
test. It might be possible to come up with a TAP test for
On 29.03.21 13:04, vignesh C wrote:
The above content looks sufficient to me.
Good, thanks. Based on that, I'm adding v7 of the patch.
Regards
Markus
From: Markus Wanner
Date: Tue, 2 Mar 2021 11:33:54 +0100
Subject: [PATCH] Add an xid argument to the filter_prepare callback for o
On 29.03.21 13:02, Ajin Cherian wrote:
Nice catch, Markus.
Interesting suggestion Amit. Let me try and code this.
Thanks, Ajin. Please consider this concurrent_abort callback as well.
I think it provides more flexibility for the output plugin and I would
therefore prefer it over a solution t
On 29.03.21 12:18, vignesh C wrote:
But in prepare_filter_cb callback, by stating "other systems ..." it is
not very clear who will change the GID. Are we referring to
publisher/subscriber decoding?
Thanks for your feedback. This is not about GIDs at all, but just about
identifying a transac
rate GID by use of
XID seems to have the same problem, so that caused confusion for me.
It was not intended as a suggestion for how to generate GIDs at all.
Hopefully leaving away that bad example will make it less likely to
appear related to GID generation on the subscriber.
Regards
Mark
On 29.03.21 11:33, Amit Kapila wrote:
You don't need an additional callback for that if we do what I am
suggesting above.
Ah, are you suggesting a different change, then? To make two-phase
transactions always send PREPARE even if concurrently aborted? In that
case, sorry, I misunderstood.
On 29.03.21 11:13, Amit Kapila wrote:
This might or might not be valid for all logical replication solutions
but in the publisher-subscriber model, it would easily lead to
duplicate identifiers and block the replication. For example, when
there are multiple subscriptions (say - 2) for multiple pu
Sorry, git tricked me. Here's the patch including actual changes.
Regards
Markus
From: Markus Wanner
Date: Tue, 2 Mar 2021 11:33:54 +0100
Subject: [PATCH] Add an xid argument to the filter_prepare callback for output plugins
---
contrib/test_decoding/test_decoding.c | 4 ++-
do
ider when using chained replication).
An updated patch is attached.
Regards
Markus
From: Markus Wanner
Date: Tue, 2 Mar 2021 11:33:54 +0100
Subject: [PATCH] Add an xid argument to the filter_prepare callback for output
plugins
---
contrib/test_decoding/test_decoding.c | 4 ++-
doc/src/sgm
On 27.03.21 07:37, Amit Kapila wrote:
Isn't it better to send prepare from the publisher in such a case so
that subscribers can know about it when rollback prepared arrives?
That's exactly what this callback allows (among other options). It
provides a way for the output plugin to react to a t
On 26.03.21 11:19, Amit Kapila wrote:
No, I am not assuming that. I am just trying to describe you that it
is not necessary that we will be able to detect concurrent abort in
each and every case.
Sure. Nor am I claiming that would be necessary or that the patch
changed anything about it.
As
On 26.03.21 04:28, Amit Kapila wrote:
I think as you have noted that stream_abort or rollback_prepared will
be sent (the remaining changes in-between will be skipped) as we
decode them from WAL
Yes, but as outlined, too late. Multiple other transactions may get
decoded until the decoder reach
On 25.03.21 21:21, Andres Freund wrote:
... the code added as part of 7259736a6e5b7c75 ...
That's the streaming part, which can be thought of as a more general
variant of the two-phase decoding in that it allows multiple "flush
points" (invoking ReorderBufferProcessTXN). Unlike the PREPARE o
k into adding
tests (concurrent aborts are not currently covered, it seems).
Regards
Markus
From: Markus Wanner
Date: Thu, 11 Feb 2021 13:49:55 +0100
Subject: [PATCH] Add a concurrent_abort callback for the output plugin.
Logical decoding of a prepared or streamed transaction may fail if
On 22.03.21 09:50, Markus Wanner wrote:
thank you for reconsidering this patch. I updated it to include the
required adjustments to the documentation. Please review.
I tweaked the wording in the docs a bit, resulting in a v3 of this patch.
Regards
Markus
From: Markus Wanner
Date: Tue, 2
From: Markus Wanner
Date: Tue, 2 Mar 2021 11:33:54 +0100
Subject: [PATCH] Add an xid argument to the filter_prepare callback for output plugins
---
contrib/test_decoding/test_decoding.c | 4 ++-
doc/src/sgml/logicaldecoding.sgml | 33 +++
src/backend/replication/lo
On 20.03.21 16:14, Amit Kapila wrote:
Right, but I guess in our case using user-provided GID will conflict
if we use multiple subscriptions on the same node. So, it is better to
generate a unique identifier like we are discussing here, something
like (origin_id of subscription + xid of the publis
On 20.03.21 03:17, Amit Kapila wrote:
Are you saying that users might use the same GID which we have
constructed internally (say by combining origin and xid: originid_xid)
and then there will be conflict while replaying such transactions?
No, I was pondering about a user doing (in short sequenc
On 18.03.21 10:45, Amit Kapila wrote:
While reviewing/testing subscriber-side work for $SUBJECT [1], I
noticed a problem that seems to need a broader discussion, so started
this thread. We can get prepare for the same GID more than once for
the cases where we have defined multiple subscriptions f
On 11.03.21 04:58, Amit Kapila wrote:
But this happens when we are decoding prepare, so it is clear that the
transaction is prepared, why any additional check?
An output plugin cannot assume the transaction is still prepared and
uncommitted at the point in time it gets to decode the prepare.
On 10.03.21 11:18, Amit Kapila wrote:
On Tue, Mar 9, 2021 at 2:14 PM Markus Wanner
wrote:
currently, only the gid is passed on to the filter_prepare callback.
While we probably should not pass a full ReorderBufferTXN (as we do for
most other output plugin callbacks), a bit more information
On 09.03.21 10:37, Amit Kapila wrote:
AFAICS, the error is removed by the patch as per below change:
Ah, well, that does not seem right, then. We cannot just silently
ignore the callback but not skip the prepare, IMO. That would lead to
the output plugin missing the PREPARE, but still seein
On 09.03.21 09:39, Amit Kapila wrote:
> It is attached with the initial email.
Oh, sorry, I looked up the initial email, but still didn't see the patch.
I think so. The behavior has to be similar to other optional callbacks
like message_cb, truncate_cb, stream_truncate_cb. Basically, we don't
n
prepare?
If you are okay with adding just the xid, I'll add docs changes to the
patch provided.
Regards
Markus
From: Markus Wanner
Date: Tue, 2 Mar 2021 11:33:54 +0100
Subject: [PATCH] Add an xid argument to the filter_prepare callback for the
output plugin.
---
contrib/test_dec
On 09.03.21 07:40, Amit Kapila wrote:
Sounds reasonable to me. I also don't see a reason why we need to make
this a necessary callback. Some plugin authors might just want 2PC
without streaming support.
Sounds okay to me. Probably means we'll have to check for this callback
and always skip th
Hello Amit,
On 04.01.21 09:18, Amit Kapila wrote:
Thanks, I have pushed the 0001* patch after making the above and a few
other cosmetic modifications.
That commit added the following snippet to the top of
ReorderBufferFinishPrepared:
txn = ReorderBufferTXNByXid(rb, xid, true, NULL, commit
On 22.02.21 05:22, Andres Freund wrote:
Hi,
On 2021-02-19 15:53:32 +0100, Markus Wanner wrote:
However, more generally speaking, I suspect you are overthinking this. All
of the complexity arises because of the assumption that an output plugin
receiving and confirming a PREPARE may not be able
On 20.02.21 13:15, Amit Kapila wrote:
I think after the patch Ajin proposed decoders won't need any special
checks after receiving the prepared xacts. What additional simplicity
this approach will bring?
The API becomes clearer in that all PREPAREs are always decoded in WAL
stream order and ar
ot the same as a state without it.
Even if the transaction is empty, or if you're actually going to roll
it back. And therefore possibly ending up at the very same state without
any useful effect.
Regards
Markus
From: Markus Wanner
Date: Sun, 21 Feb 2021 10:43:34 +0100
Subject: [PATCH] P
On 20.02.21 21:08, Andres Freund wrote:
It's not free though
Agreed. It's an additional call to a callback. Do you think that's
acceptable if limited to two-phase transactions only?
I'm wondering the opposite: What's a potential use case for handing
"trivially empty" transactions to the o
On 20.02.21 12:15, Amit Kapila wrote:
What exactly is the use case to send empty transactions with or
without prepared?
I'm not saying that output plugins should *send* empty transactions to
the replica. I rather agree that this indeed is not wanted in most cases.
However, that's not what t
On 20.02.21 04:38, Amit Kapila wrote:
I see a problem with this assumption. During the initial
synchronization, this transaction won't be visible to snapshot and we
won't copy it. Then later if we won't decode and send it then the
replica will be out of sync. Such a problem won't happen with Ajin
ee that to be possible (or how would this be
different from a regular single-phase commit?).
Please let me know what you think and whether this approach is feasible
for you as well.
Regards
Markus
>From ed03c463175733072edf8afb8d120a1285a3194f Mon Sep 17 00:00:00 2001
From: Markus Wanner
Dat
o the documentation.
Regards
Markus
From: Markus Wanner
Date: Fri, 19 Feb 2021 13:34:22 +0100
Subject: Present all committed transactions to the output plugin
Drop the filtering of some empty transactions and more
consistently present all committed transactions to the output
plugin. Changes behavior for
Hello Amit,
thanks a lot for your extensive explanation and examples, I appreciate
this very much. I'll need to think this through and see how we can make
this work for us.
Best Regards
Markus
On 10.02.21 07:32, Amit Kapila wrote:
On Wed, Feb 10, 2021 at 11:45 AM Ajin Cherian wrote:
But the other side of the problem is that ,without this, if the
prepared transaction is prior to a consistent snapshot when decoding
starts/restarts, then only the "commit prepared" is sent to downstream
Hello Amit,
thanks for your very quick response.
On 08.02.21 11:13, Amit Kapila wrote:
/*
* It is possible that this transaction is not decoded at prepare time
* either because by that time we didn't have a consistent snapshot or it
* was decoded earlier but we have restarted. We can't di
Amit, Ajin, hackers,
testing logical decoding for two-phase transactions, I stumbled over
what I first thought is a bug. But comments seems to indicate this is
intended behavior. Could you please clarify or elaborate on the design
decision? Or indicate this indeed is a bug?
What puzzled m
t holder coming forth to offer a
grant a lot better than the one who doesn't (but still holds the
patent). I'm missing the appreciation for that former strategy in this
thread and fear we're setting a precedent for the latter one, instead.
Kind Regards
--
Markus Wanner - http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
rocedure, as you seem to propose,
they'd have every right to sue not only users of derivatives, but even
Postgres users for violation of their patent.
You seem to have experience with different licences and dual licensing.
What would you recommend Takayuki-san to do?
Kind Regards
Markus
--
57 matches
Mail list logo
