Re: [WIP]Vertical Clustered Index (columnar store extension) - take2

Hi Peter!

On Wed, 2025-09-24 at 12:46 +1000, Peter Smith wrote:
> On Sat, Sep 20, 2025 at 12:13 AM Timur Magomedov
> <t.magomedov@postgrespro.ru> wrote:
> > 
> > Hi Peter!
> > 
> > > What exactly did Valgrind report? For example, you said the
> > > VciScanState points beyond allocated memory. Do you have any more
> > > clues, like where that happened? Did you discover where that
> > > (smaller
> > > than it should be) memory was allocated in the first place?
> > 
> > Doing some experiments I've faced a segfault on a query joining
> > tables
> > filled with some amount of data. It was flaky so I used Valgrind.
> > There is a line in vci_scan.c, exec_custom_plan_enabling_vp():
> > if (!scanstate->first_fetch || (scanstate->pos.num_fetched_rows <=
> > scanstate->pos.current_row))
> > Valgrind reported that line as Invalid read of size 1, 4 and 4. So
> > all
> > three of the values checked in this line are read from some random
> > memory, possibly allocated and used by other objects already.
> > 
> > When the expression in exec_custom_plan_enabling_vp() randomly
> > evaluated to true, the following ExecClearTuple() dereferences NULL
> > in
> > slot->tts_ops.
> > The memory was originally allocated in nodeHashJoin.c, in hjstate =
> > makeNode(HashJoinState) line so it is really smaller than
> > VciScanState.
> > 
> > I did not use any table data for reproducer since asserts helps to
> > catch the original problem. I also simplified the original query
> > for a
> > reproducer.
> > 
> > > OK. I am not 100% certain about the asserts, but since the
> > > existing
> > > VCI tests are passing, I have merged your patch as-is into v24-
> > > 0002.
> > > I
> > > guess we will find out later if the bug below is due to an old
> > > code
> > > cast problem or a new code assert problem.
> > > 
> > 
> > Thanks for merging asserts. And looks like the problem is related
> > to
> > VCI join nodes.
> > There is no VCI hash join or VCI nested loop. There is a code in
> > VCI
> > planner that still puts VCI Sort or VCI Aggregate nodes on top of
> > regular join nodes which makes no sense to me. This is the cause of
> > the
> > problem. VCI Sort and VCI Aggregate then convert outer nodes to VCI
> > Scan since they know there can't be anything another. This can be
> > fixed
> > either by implementing VCI joins either by disabling them in a
> > deeper
> > way. Since we already have developer GUCs for them I would rather
> > set
> > them to disabled by default instead of removing all useful VCI
> > joins
> > related code.
> > 
> > Made a patch with a test and a simplest fix (disabling joins in
> > GUCs).
> > 
> 
> Hi Timur,
> 
> Thanks for the patch! Unfortunately, this is straying into areas with
> which I'm not familiar, so I'm taking it on faith that these are good
> changes. For now, I'm happy to merge your patch into the next VCI
> version, posted unless someone else objects.
> 
> ~
> 
> But, I still have a couple of questions for clarification:
> 
> 1. What about the original Valgrind issue?
> 
> Is that still a problem that needs to be addressed? E.g., is the bad
> allocation still lurking, and your sort avoidance patch is simply
> preventing the bad allocation from being exposed until some next
> thing
> randomly fails? Or is there no allocation problem anymore to worry
> about?

Allocations are fine, the problem was using some nodes as nodes of
another type (and bigger size) which leads to crossing boundary of
allocated memory. We are safe now and asserts guard us from repeating
the original bug.


> 2. What about your added Assert that was previously failing at
> executor/vci_sort.c:89?
> 
> That Assert is still present in vci_sort.c, but AFAICT the current
> tests are not executing that code. Do those patched GUC changes
> simply
> make that code unreachable now? In other words, should that
> previously
> failing Assert be left where it is or not? Should there be another
> test case added to execute this Assert?

Added simple test for running VCI sort node, it executes the assertion
code in vci_sort.c. No assertion fails, so VCI Sort itself is OK. Here
are both two commits on top of v25 version.


-- 
Regards,
Timur Magomedov