Re: [WIP]Vertical Clustered Index (columnar store extension) - take2 - Mailing list pgsql-hackers

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?

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?

======
Kind Regards,
Peter Smith.
Fujitsu Australia