Re: Transactions involving multiple postgres foreign servers

On Thu, Jul 30, 2015 at 1:52 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Wed, Jul 29, 2015 at 6:58 AM, Ashutosh Bapat
> <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
> > A user may set atomic_foreign_transaction to ON to guarantee atomicity,
> IOW
> > it throws error when atomicity can not be guaranteed. Thus if application
> > accidentally does something to a foreign server, which doesn't support
> 2PC,
> > the transaction would abort. A user may set it to OFF (consciously and
> takes
> > the responsibility of the result) so as not to use 2PC (probably to
> reduce
> > the overheads) even if the foreign server is 2PC compliant. So, I
> thought a
> > GUC would be necessary. We can incorporate the behaviour you are
> suggesting
> > by having atomic_foreign_transaction accept three values "full" (ON
> > behaviour), "partial" (behaviour you are describing), "none" (OFF
> > behaviour). Default value of this GUC would be "partial". Will that be
> fine?
>
> I don't really see the point. If the user attempts a distributed
> transaction involving FDWs that can't support atomic foreign
> transactions, then I think it's reasonable to assume that they want
> that to work rather than arbitrarily fail. The only situation in
> which it's desirable for that to fail is when the user doesn't realize
> that the FDW in question doesn't support atomic foreign commit, and
> the error message warns them that their assumptions are unfounded.
> But can't the user find that out easily enough by reading the
> documentation? So I think that in practice the "full" value of this
> GUC would get almost zero use; I think that nearly everyone will be
> happy with what you are here calling "partial" or "none". I'll defer
> to any other consensus that emerges, but that's my view.
>
> I think that we should not change the default behavior. Currently,
> the only behavior is not to attempt 2PC. Let's stick with that.
>

Ok. I will remove the GUC and have "partial atomic" behaviour as you
suggested in previous mail.

>
> > About table level atomic commit attribute, I agree that some foreign
> tables
> > might hold "more critical" data than others from the same server, but I
> am
> > not sure whether only that attribute should dictate the atomicity or
> not. A
> > transaction collectively might need to be "atomic" even if the individual
> > tables it modified are not set atomic_commit attribute. So, we need a
> > transaction level attribute for atomicity, which may be overridden by a
> > table level attribute. Should we add support to the table level atomicity
> > setting as version 2+?
>
> I'm not hung up on the table-level attribute, but I think having a
> server-level attribute rather than a global GUC is a good idea.
> However, I welcome other thoughts on that.
>

The patch supports server level attribute. Let me repeat the relevant
description from my earlier mail
--
Every FDW needs to register the connection while starting new transaction
on a foreign connection (RegisterXactForeignServer()). A foreign server
connection is identified by foreign server oid and the local user oid
(similar to the entry cached by postgres_fdw). *While registering, FDW also
tells whether the foreign server is capable of participating in two-phase
commit protocol.* How to decide that is left entirely to the FDW. An FDW
like file_fdw may not have 2PC support at all, so all its foreign servers
do not comply with 2PC. An FDW might have all its servers 2PC compliant. An
FDW like postgres_fdw can have some of its servers compliant and some not,
depending upon server version, configuration (max_prepared_transactions =
0) etc.
--

Does that look good?

> >> We should consider other possible designs as well; the choices we make
> >> here may have a significant impact on usability.
> >
> > I looked at other RBDMSes like IBM's federated database or Oracle. They
> > support only "full" behaviour as described above with some optimizations
> > like LRO. But, I would like to hear about other options.
>
> Yes, I hope others will weigh in.
>
> >> HandleForeignTransaction is not very descriptive, and I think you're
> >> jamming together things that ought to be separated. Let's have a
> >> PrepareForeignTransaction and a ResolvePreparedForeignTransaction.
> >
> > Done, there are three hooks now
> > 1. For preparing a foreign transaction
> > 2. For resolving a prepared foreign transaction
> > 3. For committing/aborting a running foreign transaction (more
> explanation
> > later)
>
> (2) and (3) seem like the same thing. I don't see any further
> explanation later in your email; what am I missing?
>

In case of postgres_fdw, 2 always fires COMMIT/ROLLBACK PREPARED 'xyz'
(fill the prepared transaction id) and 3 always fires COMMIT/ABORT
TRANSACTION (notice absence of PREPARED and 'xyz'). We might want to
combine them into a single hook but there are slight differences there
depending upon the FDW. For postgres_fdw, 2 should get a connection which
should not have a running transaction, whereas for 3 there has to be a
running transaction on that connection. Hook 2 should get prepared foreign
transaction identifier as one of the arguments, hook 3 will not have that
argument. Hook 2 will be relevant for two-phase commit protocol where as 3
will be used for connections not using two-phase commit.

The differences are much more visible in the code.

> > IP might be fine, but consider altering dbname option or dropping it; we
> > won't find the prepared foreign transaction in new database.
>
> Probably not, but I think that's the DBA's problem, not ours.
>

Fine.

>
> > I think we
> > should at least warn the user that there exist a prepared foreign
> > transaction on given foreign server or user mapping; better even if we
> let
> > FDW decide which options are allowed to be altered when there exists a
> > foreign prepared transaction. The later requires some surgery in the way
> we
> > handle the options.
>
> We can consider that, but I don't think it's an essential part of the
> patch, and I'd punt it for now in the interest of keeping this as
> simple as possible.
>

Fine.

>
> >> Is this a change from the current behavior?
> >
> > There is no current behaviour defined per say.
>
> My point is that you had some language in the email describing what
> happens if the GUC is turned off. You shouldn't have to describe
> that, because there should be absolutely zero difference. If there
> isn't, that's a problem for this patch, and probably a subject for a
> different one.
>

Ok got it.

>
> > I have added a built-in pg_fdw_remove() (or any suitable name), which
> > removes the prepared foreign transaction entry from the memory and disk.
> The
> > function needs to be called before attempting PITR. If the recovery
> points
> > to a past time without removing file, we abort the recovery. In such
> case, a
> > DBA can remove the foreign prepared transaction file manually before
> > recovery. I have added a hint with that effect in the error message. Is
> that
> > enough?
>
> That seems totally broken. Before PITR, the database might be
> inconsistent, in which case you can't call any functions at all.
> Also, you shouldn't be trying to resolve any transactions until the
> end of recovery, because you don't know when you see that the
> transaction was prepared whether, at some subsequent time, you will
> see it resolved. You need to finish recovery and, only after entering
> normal running, decide whether to resolve any transactions that are
> still sitting around.

That's how it works in the patch for unresolved prepared foreign
transactions belonging to xids within the known range. For those belonging
to xids in future (beyond of known range of xids after PITR), we can not
determine the status of that local transaction (as those do not appear in
the xlog) and hence can not decide the fate of prepared foreign
transaction. You seem to be suggesting that we should let the recovery
finish and mark those prepared foreign transaction as "can not be resolved"
or something like that. A DBA can remove those entries once s/he has dealt
with them on the foreign server.

There's little problem with that approach. Triplet (xid, serverid, userid)
is used to identify the a foreign prepared transaction entry in memory and
is used to create unique file name for storing it on the disk. If we allow
a future xid after PITR, it might conflict with an xid of a transaction
that might take place after PITR. It will cause problem if exactly same
foreign server and user participate in the transaction with conflicting xid
(rare but possible).

Other problem is that the foreign server on which the transaction was
prepared (or the user whose mapping was used to prepare the transaction),
might have got added in a future time wrt PITR, in which case, we can not
even know which foreign server this transaction was prepared on.

> There should be no situation (short of e.g. OS
> errors writing the state files) where this stuff makes recovery fail.
>
>
During PITR, if we encounter a prepared (local) transaction with a future
xid, we just forget that prepared transaction (instead of failing
recovery). May be we should do the same for unresolved foreign prepared
transaction as well (at least for version 1); forget the unresolved
prepared foreign transactions which belong to a future xid. Anyway, as per
the timeline after PITR those never existed.

Other DBMSes solve this problem by using markers. Markers are allowed to be
set at times when there were no unresolved foreign transactions and PITR is
allowed upto those markers and not any arbitrary point in time. But this
looks out of scope of this patch.

> > I noticed that the functions pg_fdw_resolve() and pg_fdw_remove() which
> > resolve or remove unresolved prepared foreign transaction resp. are
> > effecting changes which can not be rolled back if the transaction which
> ran
> > these functions rolled back. These need to be converted into SQL command
> > like ROLLBACK PREPARED which can't be run within a transaction.
>
> Yeah, maybe. I'm not sure using a functional interface is all that
> bad, but we could think about changing it.
>
>
Fine.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company