From: | Jacob Champion <pchampion(at)vmware(dot)com> |
---|---|
To: | "zyu(at)yugabyte(dot)com" <zyu(at)yugabyte(dot)com> |
Cc: | "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, "pryzby(at)telsasoft(dot)com" <pryzby(at)telsasoft(dot)com>, "hlinnaka(at)iki(dot)fi" <hlinnaka(at)iki(dot)fi>, "sawada(dot)mshk(at)gmail(dot)com" <sawada(dot)mshk(at)gmail(dot)com>, "soumyadeep2007(at)gmail(dot)com" <soumyadeep2007(at)gmail(dot)com>, "Ashwin Agrawal (Pivotal)" <aagrawal(at)pivotal(dot)io>, "melanieplageman(at)gmail(dot)com" <melanieplageman(at)gmail(dot)com> |
Subject: | Re: Table AM modifications to accept column projection lists |
Date: | 2021-03-03 18:05:17 |
Message-ID: | b042a41f34537c1ef1058e95cb5a0e6e77d72066.camel@vmware.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, 2021-03-02 at 10:35 -0800, Zhihong Yu wrote:
> Hi,
Thanks for the review!
> + /* Make sure the the new slot is not dependent on the original tuple */
>
> There is duplicate 'the'.
Thanks, I'll add this for the next batch of updates.
> For neededColumnContextWalker(),
>
> + else if(var->varattno == 0) {
>
> I think the if following the else is not needed - I assume var->varattno wouldn't be negative.
> Similar comment for extract_scan_columns().
I think you can have system columns in the tree here -- a common
example that we run into with the `make check` suite is ctid. (To see
this, you can change the (> 0) check just above this code into a (!= 0)
check, and then take a look at the failing cases in the test suite.)
> + while ((col = bms_next_member(parent_cols, col)) >= 0)
> + {
> + Var *var = (Var *) list_nth(translated_vars, col - 1);
>
> If col is 0, do you still want to call list_nth() ?
The (col == 0) case is handled just above this, with
contains_whole_row_col() and get_ordinal_attnos() replacing any zero
columns with the entire user-column range. If one of those functions
fails to do its job due to programmer error, we'll assert in the call
to list_nth(), and I think that's what we want.
--Jacob
From | Date | Subject | |
---|---|---|---|
Next Message | Pavel Stehule | 2021-03-03 18:16:47 | Re: proposal - psql - use pager for \watch command |
Previous Message | John Naylor | 2021-03-03 18:02:18 | Re: Speeding up GIST index creation for tsvectors |