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(at)vmware(dot)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