Re: ALTER TABLE uses a bistate but not for toast tables - Mailing list pgsql-hackers

> On Wed, Nov 20, 2024 at 06:43:58AM -0600, Justin Pryzby wrote:
>
> > Thanks for rebasing. To help with review, could you also describe
> > current status of the patch? I have to admit, currently the commit
> > message doesn't tell much, and looks more like notes for the future you.
>
> The patch does what it aims to do and AFAIK in a reasonable way.  I'm
> not aware of any issue with it.  It's, uh, waiting for review.
>
> I'm happy to expand on the message to describe something like design
> choices, but the goal here is really simple: why should wide column
> values escape the intention of the ring buffer?  AFAICT it's fixing an
> omission.  If you have a question, please ask; that would help to
> indicate what needs to be explained.

Here is what I see in the commit message:

    DONE: ALTER, CLUSTER
    TODO: copyto, copyfrom?

    slot_getsomeattrs slot_deform_heap_tuple fetchatt
    heap_getnextslot => heapgettup => heapgetpage => ReadBufferExtended
    initscan
    table_beginscan table_scan_getnextslot
    RelationCopyStorageUsingBuffer ReadBufferWithoutRelcache

    (gdb) bt
     #0  table_open (relationId=relationId@entry=16390, lockmode=lockmode@entry=1) at table.c:40
     #1  0x000056444cb23d3c in toast_fetch_datum (attr=attr@entry=0x7f67933cc6cc) at detoast.c:372
     #2  0x000056444cb24217 in detoast_attr (attr=attr@entry=0x7f67933cc6cc) at detoast.c:123
     #3  0x000056444d07a4c8 in pg_detoast_datum_packed (datum=datum@entry=0x7f67933cc6cc) at fmgr.c:1743
     #4  0x000056444d042c8d in text_to_cstring (t=0x7f67933cc6cc) at varlena.c:224
     #5  0x000056444d0434f9 in textout (fcinfo=<optimized out>) at varlena.c:573
     #6  0x000056444d078f10 in FunctionCall1Coll (flinfo=flinfo@entry=0x56444e4706b0, collation=collation@entry=0,
arg1=arg1@entry=140082828592844)at fmgr.c:1124
 
     #7  0x000056444d079d7f in OutputFunctionCall (flinfo=flinfo@entry=0x56444e4706b0, val=val@entry=140082828592844)
atfmgr.c:1561
 
     #8  0x000056444ccb1665 in CopyOneRowTo (cstate=cstate@entry=0x56444e470898, slot=slot@entry=0x56444e396d20) at
copyto.c:975
     #9  0x000056444ccb2c7d in DoCopyTo (cstate=cstate@entry=0x56444e470898) at copyto.c:891
     #10 0x000056444ccab4c2 in DoCopy (pstate=pstate@entry=0x56444e396bb0, stmt=stmt@entry=0x56444e3759b0,
stmt_location=0,stmt_len=48, processed=processed@entry=0x7ffc212a6310) at copy.c:308
 

    cluster:
    heapam_relation_copy_for_cluster
    reform_and_rewrite_tuple
    rewrite_heap_tuple
    raw_heap_insert

This gave me an impression, that the patch is deeply WIP, and it doesn't
make any sense to review it. I can imagine chances are good, that I'm
not alone who get such impression, and you loose potential reviewers.
Thus, shaping up a meaningful message might be helpful.

> > Since it's in the performance category, I'm also curious how much
> > overhead does this shave off? I mean, I get it that bulk insert strategy
> > helps with buffers usage, as you've implied in the thread -- but how
> > does it look like in benchmark numbers?
>
> The intent of using a bistate isn't to help the performance of the
> process using the bistate.  Rather, the intent is to avoid harming the
> performance of other processes.  If anything, I expect it could slow
> down the process using bistate -- the same as for non-toast data.
>
> https://www.postgresql.org/message-id/CA%2BTgmobC6RD2N8kbPPTvATpUY1kisY2wJLh2jsg%3DHGoCp2RiXw%40mail.gmail.com

Right, but the question is still there, how much does it bring? My point
is that if you demonstrate "under this and that load, we get so and so
many percents boost", this will hopefully attract more attention to the
patch.