Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

On Sun, Dec 22, 2019 at 5:04 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> Few comments:
> assert variable should be within #ifdef USE_ASSERT_CHECKING in patch
> v2-0008-Add-support-for-streaming-to-built-in-replication.patch:
> + int64 subidx;
> + bool found = false;
> + char path[MAXPGPATH];
> +
> + subidx = -1;
> + subxact_info_read(MyLogicalRepWorker->subid, xid);
> +
> + /* FIXME optimize the search by bsearch on sorted data */
> + for (i = nsubxacts; i > 0; i--)
> + {
> + if (subxacts[i - 1].xid == subxid)
> + {
> + subidx = (i - 1);
> + found = true;
> + break;
> + }
> + }
> +
> + /* We should not receive aborts for unknown subtransactions. */
> + Assert(found);
>

We can use PG_USED_FOR_ASSERTS_ONLY for that variable.

>
> Should we include printing of id here like in earlier cases in
> v2-0002-Issue-individual-invalidations-with-wal_level-log.patch:
> + appendStringInfo(buf, " relcache %u", msg->rc.relId);
> + /* not expected, but print something anyway */
> + else if (msg->id == SHAREDINVALSMGR_ID)
> + appendStringInfoString(buf, " smgr");
> + /* not expected, but print something anyway */
> + else if (msg->id == SHAREDINVALRELMAP_ID)
> + appendStringInfo(buf, " relmap db %u", msg->rm.dbId);
>

I am not sure if this patch is logging these invalidations, so not
sure if it makes sense to add more ids in the cases you are referring
to. However, if we change it to logging all invalidations at command
end as being discussed in this thread, then it might be better to do
what you are suggesting.

>
> Should we can add function header for AssertChangeLsnOrder in
> v2-0006-Implement-streaming-mode-in-ReorderBuffer.patch:
> +static void
> +AssertChangeLsnOrder(ReorderBuffer *rb, ReorderBufferTXN *txn)
> +{
>
> This "Assert(txn->first_lsn != InvalidXLogRecPtr)"can be before the
> loop, can be checked only once:
> + dlist_foreach(iter, &txn->changes)
> + {
> + ReorderBufferChange *cur_change;
> +
> + cur_change = dlist_container(ReorderBufferChange,
> node, iter.cur);
> +
> + Assert(txn->first_lsn != InvalidXLogRecPtr);
> + Assert(cur_change->lsn != InvalidXLogRecPtr);
> + Assert(txn->first_lsn <= cur_change->lsn);
>

This makes sense to me. Another thing about this function, do we
really need "ReorderBuffer *rb" parameter in this function?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com