I think we need PRE_COMMIT events for (Sub)XactCallbacks - Mailing list pgsql-hackers

From Tom Lane
Subject I think we need PRE_COMMIT events for (Sub)XactCallbacks
Date
Msg-id 1142.1360885788@sss.pgh.pa.us
Whole thread Raw
Responses Re: I think we need PRE_COMMIT events for (Sub)XactCallbacks  (Simon Riggs <simon@2ndQuadrant.com>)
Re: I think we need PRE_COMMIT events for (Sub)XactCallbacks  (Pavan Deolasee <pavan.deolasee@gmail.com>)
List pgsql-hackers
I'm trying to whack the postgres_fdw patch into committable shape, with
one eye on the writable-foreign-tables patch that's right behind it in
the queue.  One thing I've come across is that the timing of remote
commits is a mess.  As-is in the submitted patches, we'll issue a commit
to the remote server as soon as the executor shuts down a query.  I'm
not too thrilled with this, since given
begin;update my_foreign_table set ... ;update my_foreign_table set ... ;update my_foreign_table set ... ;rollback;

a reasonable person would expect that the remote updates are rolled
back.  But as it stands each UPDATE is committed instantly on
completion.

The only facility that postgres_fdw would have for doing it differently
is to plug into the XactCallback or ResourceReleaseCallback hooks, and
the problem with both of those is they are post-commit hooks, which
means it's too late to throw an error.  Now foreign-side errors at
commit are not exactly difficult to foresee --- for instance, one of
our updates might have violated a deferred foreign-key constraint on the
remote side, and we won't hear about that till we try to commit.  If we
then report that error in one of the existing hooks, it'll become a
PANIC.  No good.

So I think we need to add a pre-commit event to the set of events that
XactCallbacks are called for, probably at this spot in CommitTransaction:
       /*        * Close open portals (converting holdable ones into static portals).        * If there weren't any, we
aredone ... otherwise loop back to check        * if they queued deferred triggers.  Lather, rinse, repeat.        */
   if (!PreCommit_Portals(false))           break;   }
 

+   CallXactCallbacks(XACT_EVENT_PRE_COMMIT);
+   /*    * The remaining actions cannot call any user-defined code, so it's safe    * to start shutting down
within-transactionservices.  But note that most    * of this stuff could still throw an error, which would switch us
into   * the transaction-abort path.    */
 

and similarly in PrepareTransaction.  We're probably also going to need
a pre-subcommit event in CommitSubTransaction --- maybe we could get
away without that, but I'm not convinced, and we might as well change
both the XactCallback and SubXactCallback APIs at the same time.

Note that this doesn't come near a true two-phase-commit protocol;
there will still be a window wherein we've done COMMIT on the remote
side but the local side could fail and decide to roll back.  However,
the possible errors in that window are limited and unusual, so it's
not clear to me that it's worth working harder than this.

Any objections?
        regards, tom lane



pgsql-hackers by date:

Previous
From: Tomas Vondra
Date:
Subject: Re: PATCH: Split stats file per database WAS: autovacuum stress-testing our system
Next
From: Tomas Vondra
Date:
Subject: Re: PATCH: Split stats file per database WAS: autovacuum stress-testing our system