On 2025-Mar-22, Antonin Houska wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> > I rebased this patch series; here's v09. No substantive changes from v08.
> > I made sure the tree still compiles after each commit.
I rebased again, fixing a compiler warning reported by CI and applying
pgindent to each individual patch. I'm slowly starting to become more
familiar with the whole of this new code.
> > I did look at 0002 again [...], and ended up wondering why do we need that
> > change in the first place. According to the comment where the progress
> > restore function is called, it's because reorderbuffer.c uses a
> > subtransaction internally. But I went to look at reorderbuffer.c and
> > noticed that the subtransaction is only used "when using the SQL
> > function interface, because that creates a transaction already". So
> > maybe we should look into making REPACK use reorderbuffer without having
> > to open a transaction block.
>
> Which part of reorderbuffer.c do you mean? ISTM that the use of subransactions
> is more extensive. At least ReorderBufferImmediateInvalidation() appears to
> rely on it, which in turn is called by xact_decode().
Ah, right, I was not looking hard enough. Something to keep in mind --
though I'm still not convinced that it's best to achieve this by
introducng a mechanism to restore progress state. Maybe allowing a
transaction to abort without clobbering the progress state somehow (not
trivial to implement at present though, because of layers of functions
you need to traverse with such a flag; maybe have a global in xact.c
that you set by calling a function? Not sure -- might be worse.) Not a
super critical consideration, but this point prevents me from pushing
patch 0002 here, as it may turn out that it's not needed.
But nothing prevents me from pushing 0003, so I'll see about doing that
soon, unless I see some other problem.
I also noticed that CI is complaining of a problem in Windows, which is
easily reproducible in non-Windows by defining EXEC_BACKEND. The
backtrace is this:
#0 0x000055d4fc24fe96 in hash_search (hashp=0x5606dc2a8c88, keyPtr=0x7ffeab341928, action=HASH_FIND, foundPtr=0x0)
at ../../../../../../../../../pgsql/source/master/src/backend/utils/hash/dynahash.c:960
960 return hash_search_with_hash_value(hashp,
(gdb) bt
#0 0x000055d4fc24fe96 in hash_search (hashp=0x5606dc2a8c88, keyPtr=0x7ffeab341928, action=HASH_FIND, foundPtr=0x0)
at ../../../../../../../../../pgsql/source/master/src/backend/utils/hash/dynahash.c:960
#1 0x000055d4fbea0a46 in is_concurrent_repack_in_progress (relid=21973)
at ../../../../../../../../pgsql/source/master/src/backend/commands/cluster.c:2729
#2 is_concurrent_repack_in_progress (relid=relid@entry=2964)
at ../../../../../../../../pgsql/source/master/src/backend/commands/cluster.c:2706
#3 0x000055d4fc237a87 in RelationBuildDesc (targetRelId=2964, insertIt=insertIt@entry=true)
at ../../../../../../../../../pgsql/source/master/src/backend/utils/cache/relcache.c:1257
#4 0x000055d4fc239456 in RelationIdGetRelation (relationId=<optimized out>, relationId@entry=2964)
at ../../../../../../../../../pgsql/source/master/src/backend/utils/cache/relcache.c:2105
So apparently we're trying to dereference a hash table which isn't
properly set up in the child process.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Los dioses no protegen a los insensatos. Éstos reciben protección de
otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)
