Re: Table AM modifications to accept column projection lists

On Fri, Jun 4, 2021 at 4:14 PM Jacob Champion <pchampion@vmware.com> wrote:

> On Tue, 2021-06-01 at 15:38 +0300, Aleksander Alekseev wrote:
> > I came across this patch and noticed that it rotted a little,
> > especially after removing inheritance_planner() in 86dc9005. I
> > managed to resolve the conflicts on current `master` (eb89cb43), see
> > the attached patch. The code compiles but doesn't pass the tests. I'm
> > currently in the process of reviewing it and didn't figure out what
> > the issue is yet. Just wanted to let you know.
>
> Hi Alexsander, thanks!
>
> In your patch's transformInsertStmt(), I see what I think is an
> extraneous call to transformReturningList() right before the ON
> CONFLICT processing. That call is already done later in the function,
> during the RETURNING processing (this change came in with 6c0373ab77).
> Other than that, your rebased patch looks the same as mine.
>
> >  I also believe changing the patch status to "Waiting on Author"
> > would be appropriate.
>
> 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.
>
> Thanks again!
> --Jacob
>
Hi,

+       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 ...

+   /* 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.

+typedef struct neededColumnContext
+{
+   Bitmapset **mask;
+   int n;

Should field n be named ncol ? 'n' seems too general.

+        * 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 ?

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) ?

Cheers