Re: Transactions involving multiple postgres foreign servers

On Wed, Jul 29, 2015 at 6:58 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.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.

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

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

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

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

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

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

There should be no situation (short of e.g. OS
errors writing the state files) where this stuff makes recovery fail.

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