Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions

On Mon, May 4, 2020 at 5:16 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, May 1, 2020 at 8:41 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Thu, Apr 30, 2020 at 12:31 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > >
> > > But can't they access other catalogs like pg_publication*?  I think
> > > the basic thing we want to ensure here is that all historic accesses
> > > always use systable* APIs to access catalogs.  We can ensure that via
> > > having Asserts (or elog(ERROR, ..) in heap/tableam APIs.
> >
> > Yeah, it can.  So I have changed it now, actually along with
> > CheckXidLive, I have kept one more flag so whenever CheckXidLive is
> > set and we pass through systable_beginscan we will set that flag.  So
> > while accessing the tableam API we will set if CheckXidLive is set
> > then another flag must also be set otherwise we through an error.
> >
>
> Okay, I have reviewed these changes and below are my comments:
>
> Review of  v17-0004-Gracefully-handle-concurrent-aborts-of-uncommitt
> --------------------------------------------------------------------
> 1.
> + /*
> + * If CheckXidAlive is set then set a flag that this call is passed through
> + * systable_beginscan.  See detailed  comments at snapmgr.c where these
> + * variables are declared.
> + */
> + if (TransactionIdIsValid(CheckXidAlive))
> + sysbegin_called = true;
>
> a. How about calling this variable as bsysscan or sysscan instead of
> sysbegin_called?

Done

> b. There is an extra space between detailed and comments.  A similar
> change is required at other place where this comment is used.

Done

> c. How about writing the first line as "If CheckXidAlive is set then
> set a flag to indicate that system table scan is in-progress."
>
> 2.
> -     Any actions leading to transaction ID assignment are prohibited.
> That, among others,
> -     includes writing to tables, performing DDL changes, and
> -     calling <literal>pg_current_xact_id()</literal>.
> +     Note that access to user catalog tables or regular system
> catalog tables in
> +     the output plugins has to be done via the
> <literal>systable_*</literal> scan
> +     APIs only. The user tables should not be accesed in the output
> plugins anyways.
> +     Access via the <literal>heap_*</literal> scan APIs will error out.
>
> The line "The user tables should not be accesed in the output plugins
> anyways." seems a bit of out of place.  I don't think this is required
> here.  If you read the previous paragraph in the same document it is
> written: "Read only access to relations is permitted as long as only
> relations are accessed that either have been created by
> <command>initdb</command> in the <literal>pg_catalog</literal> schema,
> or have been marked as user provided catalog tables using ...".  I
> think that is sufficient to convey the information that the newly
> added line by you is trying to convey.

Right.

>
> 3.
> + /*
> + * We don't expect direct calls to this routine when CheckXidAlive is a
> + * valid transaction id, this should only come through systable_* call.
> + * CheckXidAlive is set during logical decoding of a transactions.
> + */
> + if (unlikely(TransactionIdIsValid(CheckXidAlive) && !sysbegin_called))
> + elog(ERROR, "unexpected heap_getnext call during logical decoding");
>
> How about changing this comment as "We don't expect direct calls to
> heap_getnext with valid CheckXidAlive for catalog or regular tables.
> See detailed comments at snapmgr.c where these variables are
> declared."?  Change the similar comment used in other places in the
> patch.
>
> For this specific API, we can also say "Normally we have such a check
> at tableam level API but this is called from many places so we need to
> ensure it here."

Done

>
> 4.
> + * If CheckXidAlive is valid, then we check if it aborted. If it did, we error
> + * out.  We can't directly use TransactionIdDidAbort as after crash such
> + * transaction might not have been marked as aborted.  See detailed  comments
> + * at snapmgr.c where the variable is declared.
> + */
> +static inline void
> +HandleConcurrentAbort()
>
> Can we change the comments as "Error out, if CheckXidAlive is aborted.
> We can't directly use TransactionIdDidAbort as after crash such
> transaction might not have been marked as aborted."
>
> After this add one empty line and then we can say something like:
> "This is a special API to check if CheckXidAlive is aborted in system
> table scan APIs.  See detailed comments at snapmgr.c where the
> variable is declared."
>
> 5. Shouldn't we add a check in table_scan_sample_next_block and
> table_scan_sample_next_tuple APIs as well?

Done

> 6.
> /*
> + * An xid value pointing to a possibly ongoing (sub)transaction.
> + * Currently used in logical decoding.  It's possible that such transactions
> + * can get aborted while the decoding is ongoing.  If CheckXidAlive is set
> + * then we will set sysbegin_called flag when we call systable_beginscan.  This
> + * is to ensure that from the pgoutput plugin we should never directly access
> + * the tableam or heap apis because we are checking for the concurrent abort
> + * only in systable_* apis.
> + */
> +TransactionId CheckXidAlive = InvalidTransactionId;
> +bool sysbegin_called = false;
>
> Can we change the above comment as "CheckXidAlive is a xid value
> pointing to a possibly ongoing (sub)transaction.  Currently, it is
> used in logical decoding.  It's possible that such transactions can
> get aborted while the decoding is ongoing in which case we skip
> decoding that particular transaction. To ensure that we check whether
> the CheckXidAlive is aborted after fetching the tuple from system
> tables.  We also ensure that during logical decoding we never directly
> access the tableam or heap APIs because we are checking for the
> concurrent aborts only in systable_* APIs."

Done

I have also fixed one issue in the patch
v18-0010-Bugfix-handling-of-incomplete-toast-tuple.patch.

Basically, the check, in ReorderBufferLargestTopTXN for selecting the
largest top transaction was incorrect so I have fixed that.


--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com