On Tue, Jan 29, 2019 at 5:47 PM Ildar Musin <ildar@adjust.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
