Re: Perform streaming logical transactions by background workers and parallel apply

On Wed, Jul 27, 2022 at 1:27 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Wednesday, July 27, 2022 1:29 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Wed, Jul 27, 2022 at 10:06 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> > >
> > > What kind of failure do you have in mind and how it can occur? The one
> > > way it can fail is if the publisher doesn't have a corresponding
> > > foreign key on the table because then the publisher could have allowed
> > > an insert into a table (insert into FK table without having the
> > > corresponding key in PK table) which may not be allowed on the
> > > subscriber. However, I don't see any check that could prevent this
> > > because for this we need to compare the FK list for a table from the
> > > publisher with the corresponding one on the subscriber. I am not
> > > really sure if due to the risk of such conflicts we should block
> > > parallelism of transactions operating on tables with FK because those
> > > conflicts can occur even without parallelism, it is just a matter of
> > > timing. But, I could be missing something due to which the above check
> > > can be useful?
> >
> > Actually, my question starts with this check[1][2], from this it
> > appears that if this relation is having a foreign key then we are
> > marking it parallel unsafe[2] and later in [1] while the worker is
> > applying changes for that relation and if it was marked parallel
> > unsafe then we are throwing error. So my question was why we are
> > putting this restriction? Although this error is only talking about
> > unique and non-immutable functions this is also giving an error if the
> > target table had a foreign key. So my question was do we really need
> > to restrict this? I mean why we are restricting this case?
> >
>
> Hi,
>
> I think the foreign key check is used to prevent the apply worker from waiting
> indefinitely which is caused by foreign key difference between publisher and
> subscriber, Like the following example:
>
> -------------------------------------
> Publisher:
> -- both table are published
> CREATE TABLE PKTABLE ( ptest1 int);
> CREATE TABLE FKTABLE ( ftest1 int);
>
> -- initial data
> INSERT INTO PKTABLE VALUES(1);
>
> Subcriber:
> CREATE TABLE PKTABLE ( ptest1 int PRIMARY KEY);
> CREATE TABLE FKTABLE ( ftest1 int REFERENCES PKTABLE);
>
> -- Execute the following transactions on publisher
>
> Tx1:
> INSERT ... -- make enough changes to start streaming mode
> DELETE FROM PKTABLE;
> Tx2:
> INSERT ITNO FKTABLE VALUES(1);
> COMMIT;
> COMMIT;
> -------------------------------------
>
> The subcriber's apply worker will wait indefinitely, because the main apply worker is
> waiting for the streaming transaction to finish which is in another apply
> bgworker.
>

IIUC, here the problem will be that TX2 (Insert in FK table) performed
by the apply worker will wait for a parallel worker doing streaming
transaction TX1 which has performed Delete from PK table. This wait is
required because we can't decide if Insert will be successful or not
till TX1 is either committed or Rollback. This is similar to the
problem related to primary/unique keys mentioned earlier [1]. If so,
then, we should try to forbid this in some way to avoid subscribers
from being stuck.

Dilip, does this reason sounds sufficient to you for such a check, or
do you still think we don't need any check for FK's?

>
> BTW, I think the foreign key won't take effect in subscriber's apply worker by
> default. Because we set session_replication_role to 'replica' in apply worker
> which prevent the FK trigger function to be executed(only the trigger with
> FIRES_ON_REPLICA flag will be executed in this mode). User can only alter the
> trigger to enable it on replica mode to make the foreign key work. So, ISTM, we
> won't hit this ERROR frequently.
>
> And based on this, another comment about the patch is that it seems unnecessary
> to directly check the FK returned by RelationGetFKeyList. Checking the actual FK
> trigger function seems enough.
>

That is correct. I think it would have been better if we can detect
that publisher doesn't have FK but the subscriber has FK as it can
occur only in that scenario. If that requires us to send more
information from the publisher, we can leave it for now (as this
doesn't seem to be a frequent scenario) and keep a simpler check based
on subscriber schema.

I think we should add a test as mentioned by you above so that if
tomorrow one tries to remove the FK check, we have a way to know.
Also, please add comments and tests for additional checks related to
constraints in the patch.

[1] - https://www.postgresql.org/message-id/CAA4eK1JwahU_WuP3S%2B7POqta%3DPhm_3gxZeVmJuuoUq1NV%3DkrXA%40mail.gmail.com

--
With Regards,
Amit Kapila.