Re: Table AM modifications to accept column projection lists

On Sat, 2021-06-05 at 09:47 -0700, Zhihong Yu wrote:
> On Fri, Jun 4, 2021 at 4:14 PM Jacob Champion <pchampion@vmware.com> wrote:
> > Agreed. I'm going to double-check with Deep that the new calls
> > to table_tuple_fetch_row_version() should be projecting the full row,
> > then post an updated patch some time next week.

(The discussions over the fallout of the inheritance_planner fallout
are still going, but in the meantime here's an updated v4 that builds
and passes `make check`.)

> +       return relation->rd_tableam->scan_begin_with_column_projection(relation, snapshot, 0, NULL,
> +                                           parallel_scan, flags, proj);
> 
> scan_begin_with_column_projection() adds a parameter to scan_begin().
> Can scan_begin() be enhanced with this projection parameter ?
> Otherwise in the future we may have scan_begin_with_column_projection_with_x_y ...

Maybe; I agree that would match the current "extension" APIs a little
better. I'll let Deep and/or Ashwin chime in on why this design was
chosen.

> +   /* Make sure the the new slot is not dependent on the original tuple */
> 
> Double 'the' in the comment. More than one place with duplicate 'the'
> in the patch.

Fixed.

> +typedef struct neededColumnContext
> +{
> +   Bitmapset **mask;
> +   int n;
> 
> Should field n be named ncol ? 'n' seems too general.

Agreed; changed to ncol.

> +        * TODO: Remove this hack!! This should be done once at the start of the tid scan.
> 
> Would the above be addressed in the next patch ?

I have not had time to get to this in v4, sorry.

> Toward the end of extract_scan_columns():
> 
> +                       bms_free(rte->scanCols);
> +                       rte->scanCols = bms_make_singleton(0);
> +                       break;
> 
> Should 'goto outer;' be in place of 'break;' (since rte->scanCols has
> been assigned for whole-row) ?

Agreed and fixed. Thank you!

--Jacob