Re: Table AM modifications to accept column projection lists

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