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

On Tue, May 5, 2020 at 10:25 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, May 5, 2020 at 9:27 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Mon, May 4, 2020 at 5:16 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Fri, May 1, 2020 at 8:41 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > >
> >
> > > 5. Shouldn't we add a check in table_scan_sample_next_block and
> > > table_scan_sample_next_tuple APIs as well?
> >
> > I am not sure that we need to do that, Because generally, we want to
> > avoid getting any wrong system table tuple which we can use for taking
> > some decision or decode tuple. But, I don't think that
> > table_scan_sample falls under that category.
> >
>
> Hmm, I am asking a check similar to what you have in function
> table_scan_bitmap_next_block(), can't we have that one?

Yeah we can put that and there is no harm in that, but my point is
the table_scan_bitmap_next_block and other functions where I have put
the check are used for fetching the tuple which can be used for
decoding tuple or taking some decision, but IMHO,
table_scan_sample_next_tuple is only used for analyzing the table. So
do we really need to do that? Am I missing something here?

BTW, I
> noticed a below spurious line removal in the patch we are talking
> about.
>
> +/*
> * These are updated by GetSnapshotData. We initialize them this way
> * for the convenience of TransactionIdIsInProgress: even in bootstrap
> * mode, we don't want it to say that BootstrapTransactionId is in progress.
> @@ -2043,7 +2055,6 @@ SetupHistoricSnapshot(Snapshot
> historic_snapshot, HTAB *tuplecids)
> tuplecid_data = tuplecids;
> }
>
> -

Okay, I will take care. of this.

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