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