locking [user] catalog tables vs 2pc vs logical rep

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

locking [user] catalog tables vs 2pc vs logical rep

Andres Freund
Hi,

The 2pc decoding added in

commit a271a1b50e9bec07e2ef3a05e38e7285113e4ce6
Author: Amit Kapila <[hidden email]>
Date:   2021-01-04 08:34:50 +0530

    Allow decoding at prepare time in ReorderBuffer.

has a deadlock danger when used in a way that takes advantage of
separate decoding of the 2PC PREPARE.


I assume the goal of decoding the 2PC PREPARE is so one can wait for the
PREPARE to have logically replicated, before doing the COMMIT PREPARED.


However, currently it's pretty easy to get into a state where logical
decoding cannot progress until the 2PC transaction has
committed/aborted. Which essentially would lead to undetected deadlocks.

The problem is that catalog tables accessed during logical decoding need
to get locked (otherwise e.g. a table rewrite could happen
concurrently). But if the prepared transaction itself holds a lock on a
catalog table, logical decoding will block on that lock - which won't be
released until replication progresses. A deadlock.

A trivial example:

SELECT pg_create_logical_replication_slot('test', 'test_decoding');
CREATE TABLE foo(id serial primary key);
BEGIN;
LOCK pg_class;
INSERT INTO foo DEFAULT VALUES;
PREPARE TRANSACTION 'foo';

-- hangs waiting for pg_class to be unlocked
SELECT pg_logical_slot_get_changes('test', NULL, NULL, 'two-phase-commit', '1');


Now, more realistic versions of this scenario would probably lock a
'user catalog table' containing replication metadata instead of
pg_class, but ...


At first this seems to be a significant issue. But on the other hand, if
you were to shut the cluster down in this situation (or disconnect all
sessions), you have broken cluster on your hand - without logical
decoding being involved. As it turns out, we need to read pg_class to
log in...  And I can't remember this being reported to be a problem?


Perhaps all that we need to do is to disallow 2PC prepare if [user]
catalog tables have been locked exclusively? Similar to how we're
disallowing preparing tables with temp table access.


Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: locking [user] catalog tables vs 2pc vs logical rep

akapila
On Tue, Feb 23, 2021 at 3:58 AM Andres Freund <[hidden email]> wrote:
>
>
> At first this seems to be a significant issue. But on the other hand, if
> you were to shut the cluster down in this situation (or disconnect all
> sessions), you have broken cluster on your hand - without logical
> decoding being involved. As it turns out, we need to read pg_class to
> log in...  And I can't remember this being reported to be a problem?
>

I don't remember seeing such a report but I think that is a reason
enough (leaving aside logical decoding of 2PC) to either disallow
locking catalog tables or at least document it in some way.

>
> Perhaps all that we need to do is to disallow 2PC prepare if [user]
> catalog tables have been locked exclusively?
>

Right, and we have discussed this during development [1][2]. We
thought either we disallow this operation or will document it. I
thought of doing this along with a core-implementation of Prepare
waiting to get it logically replicated. But at this stage, I think if
the user wants he can do a similar thing in his application where
after prepare it can wait for the transaction to get logically
replicated (if they have their own replication solution based on
logical decoding) and then decide whether to rollback or commit. So,
maybe we should either disallow this operation or at least document
it. What do you think?

> Similar to how we're
> disallowing preparing tables with temp table access.
>

Yeah, we disallow other things like pg_export_snapshot as well in a
Prepared transaction, so we can probably disallow this operation as
well.

[1] - https://www.postgresql.org/message-id/CAA4eK1JeeXOwD6rYnhSOYk5YN-fUTmxe1GkTpN2-BvgnKN6gZg%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CAMGcDxf83P5SGnGH52=_0wRP9pO6uRWCMRwAA0nxKtZvir2_vQ@...

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

Re: locking [user] catalog tables vs 2pc vs logical rep

Andres Freund
Hi,

On 2021-02-23 08:56:39 +0530, Amit Kapila wrote:
> On Tue, Feb 23, 2021 at 3:58 AM Andres Freund <[hidden email]> wrote:
> > Perhaps all that we need to do is to disallow 2PC prepare if [user]
> > catalog tables have been locked exclusively?

> Right, and we have discussed this during development [1][2].

I remember bringing it up before as well... Issues like this really need
to be mentioned as explicit caveats at least somewhere in the code and
commit message. You can't expect people to look at 3+ year old threads.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: locking [user] catalog tables vs 2pc vs logical rep

akapila
On Tue, Feb 23, 2021 at 9:09 AM Andres Freund <[hidden email]> wrote:

>
> On 2021-02-23 08:56:39 +0530, Amit Kapila wrote:
> > On Tue, Feb 23, 2021 at 3:58 AM Andres Freund <[hidden email]> wrote:
> > > Perhaps all that we need to do is to disallow 2PC prepare if [user]
> > > catalog tables have been locked exclusively?
>
> > Right, and we have discussed this during development [1][2].
>
> I remember bringing it up before as well... Issues like this really need
> to be mentioned as explicit caveats at least somewhere in the code and
> commit message.
>

Okay, so is it sufficient to add comments in code, or do we want to
add something in docs? I am not completely sure if we need to add in
docs till we have core-implementation of prepare waiting to get
logically replicated.

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

Re: locking [user] catalog tables vs 2pc vs logical rep

Andres Freund
Hi

On 2021-02-23 09:24:18 +0530, Amit Kapila wrote:
> Okay, so is it sufficient to add comments in code, or do we want to
> add something in docs? I am not completely sure if we need to add in
> docs till we have core-implementation of prepare waiting to get
> logically replicated.

There's plenty users of logical decoding that aren't going through the
normal replication mechanism - so they can hit this. So I think it needs
to be documented somewhere.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: locking [user] catalog tables vs 2pc vs logical rep

akapila
On Tue, Feb 23, 2021 at 9:33 AM Andres Freund <[hidden email]> wrote:

>
> On 2021-02-23 09:24:18 +0530, Amit Kapila wrote:
> > Okay, so is it sufficient to add comments in code, or do we want to
> > add something in docs? I am not completely sure if we need to add in
> > docs till we have core-implementation of prepare waiting to get
> > logically replicated.
>
> There's plenty users of logical decoding that aren't going through the
> normal replication mechanism - so they can hit this. So I think it needs
> to be documented somewhere.
>
As per discussion, the attached patch updates both docs and comments
in the code.

--
With Regards,
Amit Kapila.

v1-0001-Update-the-docs-and-comments-for-decoding-of-prep.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: locking [user] catalog tables vs 2pc vs logical rep

akapila
On Tue, Feb 23, 2021 at 12:00 PM Amit Kapila <[hidden email]> wrote:

>
> On Tue, Feb 23, 2021 at 9:33 AM Andres Freund <[hidden email]> wrote:
> >
> > On 2021-02-23 09:24:18 +0530, Amit Kapila wrote:
> > > Okay, so is it sufficient to add comments in code, or do we want to
> > > add something in docs? I am not completely sure if we need to add in
> > > docs till we have core-implementation of prepare waiting to get
> > > logically replicated.
> >
> > There's plenty users of logical decoding that aren't going through the
> > normal replication mechanism - so they can hit this. So I think it needs
> > to be documented somewhere.
> >
>
> As per discussion, the attached patch updates both docs and comments
> in the code.
>

I have pushed this patch.

--
With Regards,
Amit Kapila.