Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions - Mailing list pgsql-hackers

On Mon, Dec 2, 2019 at 3:32 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> I have rebased the patch set on the latest head.

0001 looks like a clever approach, but are you sure it doesn't hurt
performance when many small XLOG records are being inserted? I think
XLogRecordAssemble() can get pretty hot in some workloads.

With regard to 0002, logging a separate WAL record for each
invalidation seems painful; I think most operations that generate
invalidations generate a bunch of them all at once. Perhaps you could
just queue up invalidations as they happen, and then force anything
that's been queued up to be emitted into WAL just before you emit any
WAL record that might need to be decoded.

Regarding 0005, it seems to me that this is no good:

+ errmsg("improper heap_getnext call")));

I think we should be using elog() rather than ereport() here, because
this should only happen if there's a bug in a logical decoding plugin.
At first, I thought maybe this should just be an Assert(), but since
there are third-party logical decoding plugins available, checking
this even in non-assert builds seems like a good idea. However, I
think making it translatable is overkill; users should never see this,
only developers.

I also think that the message is really bad, because it just tells you
did something bad. It gives no inkling as to why it was bad.

0006 contains lots of XXX comments that look like real issues. I guess
those need to be fixed. Also, why don't we do the thing that the
commit message for 0006 says we could "theoretically" do? I don't
understand why we need the k-way merge at all,

+ if (prev_lsn != InvalidXLogRecPtr)
+ Assert(prev_lsn <= change->lsn);

There is no reason to ever write an if statement that contains only an
Assert, and it's bad style. Write Assert(prev_lsn == InvalidXLogRecPtr
|| prev_lsn <= change->lsn), or better yet, use XLogRecPtrIsInvalid.

The purpose and mechanism of the is_schema_sent flag is not clear to
me. The word "schema" here seems to be being used to mean "snapshot,"
which is rather confusing.

I'm also somewhat unclear on what's happening here with invalidations.
Perhaps that's as much a defect in my understanding as it is
reflective of any problem with the patch, but I also don't see any
comments either in 0002 or later patches explaining the theory of
operation. If I've missed some, please point me in the right
direction. Hypothetically speaking, it seems to me that if you just
did InvalidateSystemCaches() every time the snapshot changed, you
wouldn't need anything else (unless we're concerned with
non-transactional invalidation messages like smgr and relmapper
invalidations; not quite sure how those are handled). And, on the
other hand, if we don't do InvalidateSystemCaches() every time the
snapshot changes, then I don't understand why this works now, even
without streaming.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company