Re: Transactions involving multiple postgres foreign servers, take 2

On Tue, 14 Jul 2020 at 09:08, Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote:
>
> > I've attached the latest version patches. I've incorporated the review
> > comments I got so far and improved locking strategy.
>
> Thanks for updating the patch!
> I have three questions about the v23 patches.
>
>
> 1. messages related to user canceling
>
> In my understanding, there are two messages
> which can be output when a user cancels the COMMIT command.
>
> A. When prepare is failed, the output shows that
>     committed locally but some error is occurred.
>
> ```
> postgres=*# COMMIT;
> ^CCancel request sent
> WARNING:  canceling wait for resolving foreign transaction due to user
> request
> DETAIL:  The transaction has already committed locally, but might not
> have been committed on the foreign server.
> ERROR:  server closed the connection unexpectedly
>          This probably means the server terminated abnormally
>          before or while processing the request.
> CONTEXT:  remote SQL command: PREPARE TRANSACTION
> 'fx_1020791818_519_16399_10'
> ```
>
> B. When prepare is succeeded,
>     the output show that committed locally.
>
> ```
> postgres=*# COMMIT;
> ^CCancel request sent
> WARNING:  canceling wait for resolving foreign transaction due to user
> request
> DETAIL:  The transaction has already committed locally, but might not
> have been committed on the foreign server.
> COMMIT
> ```
>
> In case of A, I think that "committed locally" message can confuse user.
> Because although messages show committed but the transaction is
> "ABORTED".
>
> I think "committed" message means that "ABORT" is committed locally.
> But is there a possibility of misunderstanding?

No, you're right. I'll fix it in the next version patch.

I think synchronous replication also has the same problem. It says
"the transaction has already committed" but it's not true when
executing ROLLBACK PREPARED.

BTW how did you test the case (A)? It says canceling wait for foreign
transaction resolution but the remote SQL command is PREPARE
TRANSACTION.

>
> In case of A, it's better to change message for user friendly, isn't it?
>
>
> 2. typo
>
> Is trasnactions in fdwxact.c typo?
>

Fixed.

>
> 3. FdwXactGetWaiter in fdwxact.c return unused value
>
> FdwXactGetWaiter is called in FXRslvLoop function.
> It returns *waitXid_p, but FXRslvloop doesn't seem to
> use *waitXid_p. Do we need to return it?

Removed.

I've incorporated the above your comments in the local branch. I'll
post the latest version patch after incorporating other comments soon.

Regards,

-- 
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services