Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2

On Tue, Jan 29, 2019 at 5:47 PM Ildar Musin <ildar(at)adjust(dot)com> wrote:
>
> Hello,
>
> The patch needs rebase as it doesn't apply to the current master. I applied it
> to the older commit to test it. It worked fine so far.

Thank you for testing the patch!

>
> I found one bug though which would cause resolver to finish by timeout even
> though there are unresolved foreign transactions in the list. The
> `fdw_xact_exists()` function expects database id as the first argument and xid
> as the second. But everywhere it is called arguments specified in the different
> order (xid first, then dbid). Also function declaration in header doesn't
> match its definition.

Will fix.

>
> There are some other things I found.
> * In `FdwXactResolveAllDanglingTransactions()` variable `n_resolved` is
> declared as bool but used as integer.
> * In fdwxact.c's module comment there are `FdwXactRegisterForeignTransaction()`
> and `FdwXactMarkForeignTransactionModified()` functions mentioned that are
> not there anymore.
> * In documentation (storage.sgml) there is no mention of `pg_fdw_xact`
> directory.
>
> Couple of stylistic notes.
> * In `FdwXactCtlData struct` there are both camel case and snake case naming
> used.
> * In `get_fdw_xacts()` `xid != InvalidTransactionId` can be replaced with
> `TransactionIdIsValid(xid)`.
> * In `generate_fdw_xact_identifier()` the `fx` prefix could be a part of format
> string instead of being processed by `sprintf` as an extra argument.
>

I'll incorporate them at the next patch set.

> I'll continue looking into the patch. Thanks!

Thanks. Actually I'm updating the patch set, changing API interface as
I proposed before and improving the document and README. I'll submit
the latest patch next week.

--
Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center