Horton

Thread

Commits

  1. Further fixing for multi-row VALUES lists for updatable views.

  2. Fix DEFAULT-handling in multi-row VALUES lists for updatable views.

  1. BUG #15623: Inconsistent use of default for updatable view

    The Post Office <noreply@postgresql.org> — 2019-02-07T21:42:30Z

    The following bug has been logged on the website:

Bug reference:      15623
Logged by:          Roger Curley
Email address:      rocurley@gmail.com
PostgreSQL version: 11.1
Operating system:   Ubuntu 11.1
Description:        

Steps to reproduce (run in psql shell):
```
DROP TABLE IF EXISTS test CASCADE;
CREATE TABLE test (
  id int PRIMARY KEY,
  value int DEFAULT 0
);
CREATE VIEW test_view AS (SELECT * FROM test);

INSERT INTO test_view VALUES (1, DEFAULT), (2, DEFAULT);
INSERT INTO test VALUES (3, DEFAULT), (4, DEFAULT);
INSERT INTO test_view VALUES (5, DEFAULT);
SELECT * FROM test;
```

Result:
```
 id | value 
----+-------
  1 |      
  2 |      
  3 |     0
  4 |     0
  5 |     0
```

Expected Result:
```
 id | value 
----+-------
  1 |     0
  2 |     0
  3 |     0
  4 |     0
  5 |     0
```
In particular, it's surprising that inserting 1 row into an updatable view
uses the table default, while inserting 2 uses null.

  2. Re: BUG #15623: Inconsistent use of default for updatable view

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-02-08T05:07:28Z

    Hi,

On 2019/02/08 6:42, PG Bug reporting form wrote:
> The following bug has been logged on the website:
> 
> Bug reference:      15623
> Logged by:          Roger Curley
> Email address:      rocurley@gmail.com
> PostgreSQL version: 11.1
> Operating system:   Ubuntu 11.1
> Description:        
> 
> Steps to reproduce (run in psql shell):
> ```
> DROP TABLE IF EXISTS test CASCADE;
> CREATE TABLE test (
>   id int PRIMARY KEY,
>   value int DEFAULT 0
> );
> CREATE VIEW test_view AS (SELECT * FROM test);
> 
> INSERT INTO test_view VALUES (1, DEFAULT), (2, DEFAULT);
> INSERT INTO test VALUES (3, DEFAULT), (4, DEFAULT);
> INSERT INTO test_view VALUES (5, DEFAULT);
> SELECT * FROM test;
> ```
> 
> Result:
> ```
>  id | value 
> ----+-------
>   1 |      
>   2 |      
>   3 |     0
>   4 |     0
>   5 |     0
> ```
> 
> Expected Result:
> ```
>  id | value 
> ----+-------
>   1 |     0
>   2 |     0
>   3 |     0
>   4 |     0
>   5 |     0
> ```
> In particular, it's surprising that inserting 1 row into an updatable view
> uses the table default, while inserting 2 uses null.

Thanks for the report.  Seems odd indeed.

Looking into this, the reason it works when inserting just one row vs.
more than one row is that those two cases are handled by nearby but
different pieces of code.  The code that handles multiple rows seems buggy
as seen in the above example.  Specifically, I think the bug is in
rewriteValuesRTE() which is a function to replace the default placeholders
in the input rows by the default values as defined for the target
relation.  It is called twice when inserting via the view -- first for the
view relation and then again for the underlying table.  This arrangement
seems to work correctly if the view specifies its own defaults for columns
(assuming that it's okay for the view's defaults to override the
underlying base table's).  If there are no view-specified defaults, then
rewriteValuesRTE replaces the default placeholders in the input row by
NULL constants when called for the first time with the view as target
relation and the next invocation for the underlying table finds that it
has no work to do, so its defaults are not filled.

Attached find a patch that adjusts rewriteValuesRTE to not replace the
default placeholder if the view has no default value for a given column.
Also, adds a test in updatable_views.sql.

Thanks,
Amit

  3. Re: BUG #15623: Inconsistent use of default for updatable view

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2019-02-08T11:00:47Z

    On Fri, 8 Feb 2019 at 05:07, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Thanks for the report.  Seems odd indeed.

Hmm, indeed. That seems to have been broken ever since updatable views
were added.

> Looking into this, the reason it works when inserting just one row vs.
> more than one row is that those two cases are handled by nearby but
> different pieces of code.  The code that handles multiple rows seems buggy
> as seen in the above example.  Specifically, I think the bug is in
> rewriteValuesRTE() which is a function to replace the default placeholders
> in the input rows by the default values as defined for the target
> relation.  It is called twice when inserting via the view -- first for the
> view relation and then again for the underlying table.

Right, except when the view is trigger-updatable. In that case, we do
have to explicitly set the column value to NULL when
rewriteValuesRTE() is called for the view, because it won't be called
again for the underlying table -- it is the trigger's responsibility
to work how (or indeed if) to update the underlying table. IOW, you
need to also use view_has_instead_trigger() to check the view,
otherwise your patch breaks this case:

DROP TABLE IF EXISTS test CASCADE;
CREATE TABLE test (
  id int PRIMARY KEY,
  value int DEFAULT 0
);
CREATE VIEW test_view AS (SELECT * FROM test);

CREATE OR REPLACE FUNCTION test_view_ins() RETURNS trigger
AS
$$
BEGIN
  INSERT INTO test VALUES (NEW.id, NEW.value);
  RETURN NEW;
END;
$$
LANGUAGE plpgsql;

CREATE TRIGGER test_view_trig INSTEAD OF INSERT ON test_view
  FOR EACH ROW EXECUTE FUNCTION test_view_ins();

INSERT INTO test_view VALUES (1, DEFAULT), (2, DEFAULT);

ERROR:  unrecognized node type: 142


While playing around with this, I noticed a related bug affecting the
new identity columns feature. I've not investigated it fully, but It
looks almost the same -- if the column is an identity column, and
we're inserting a multi-row VALUES set containing DEFAULTS, they will
get rewritten to NULLs which will then lead to an error if overriding
the generated value isn't allowed:

DROP TABLE IF EXISTS foo CASCADE;
CREATE TABLE foo
(
  a int,
  b int GENERATED ALWAYS AS IDENTITY
);

INSERT INTO foo VALUES (1,DEFAULT); -- OK
INSERT INTO foo VALUES (2,DEFAULT),(3,DEFAULT); -- Fails

I think fixing that should be tackled separately, because it may turn
out to be subtly different, but it definitely looks like another bug.

Regards,
Dean

  4. Re: BUG #15623: Inconsistent use of default for updatable view

    Amit Langote <amitlangote09@gmail.com> — 2019-02-09T16:57:16Z

    Thanks for looking at this.

On Fri, Feb 8, 2019 at 8:01 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Fri, 8 Feb 2019 at 05:07, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> > Thanks for the report.  Seems odd indeed.
>
> Hmm, indeed. That seems to have been broken ever since updatable views
> were added.
>
> > Looking into this, the reason it works when inserting just one row vs.
> > more than one row is that those two cases are handled by nearby but
> > different pieces of code.  The code that handles multiple rows seems buggy
> > as seen in the above example.  Specifically, I think the bug is in
> > rewriteValuesRTE() which is a function to replace the default placeholders
> > in the input rows by the default values as defined for the target
> > relation.  It is called twice when inserting via the view -- first for the
> > view relation and then again for the underlying table.
>
> Right, except when the view is trigger-updatable. In that case, we do
> have to explicitly set the column value to NULL when
> rewriteValuesRTE() is called for the view, because it won't be called
> again for the underlying table -- it is the trigger's responsibility
> to work how (or indeed if) to update the underlying table. IOW, you
> need to also use view_has_instead_trigger() to check the view,
> otherwise your patch breaks this case:
>
> DROP TABLE IF EXISTS test CASCADE;
> CREATE TABLE test (
>   id int PRIMARY KEY,
>   value int DEFAULT 0
> );
> CREATE VIEW test_view AS (SELECT * FROM test);
>
> CREATE OR REPLACE FUNCTION test_view_ins() RETURNS trigger
> AS
> $$
> BEGIN
>   INSERT INTO test VALUES (NEW.id, NEW.value);
>   RETURN NEW;
> END;
> $$
> LANGUAGE plpgsql;
>
> CREATE TRIGGER test_view_trig INSTEAD OF INSERT ON test_view
>   FOR EACH ROW EXECUTE FUNCTION test_view_ins();
>
> INSERT INTO test_view VALUES (1, DEFAULT), (2, DEFAULT);
>
> ERROR:  unrecognized node type: 142

Oops, I missed this bit.  Updated the patch per your suggestion and
expanded the test case to exercise this.

> While playing around with this, I noticed a related bug affecting the
> new identity columns feature. I've not investigated it fully, but It
> looks almost the same -- if the column is an identity column, and
> we're inserting a multi-row VALUES set containing DEFAULTS, they will
> get rewritten to NULLs which will then lead to an error if overriding
> the generated value isn't allowed:
>
> DROP TABLE IF EXISTS foo CASCADE;
> CREATE TABLE foo
> (
>   a int,
>   b int GENERATED ALWAYS AS IDENTITY
> );
>
> INSERT INTO foo VALUES (1,DEFAULT); -- OK
> INSERT INTO foo VALUES (2,DEFAULT),(3,DEFAULT); -- Fails
>
> I think fixing that should be tackled separately, because it may turn
> out to be subtly different, but it definitely looks like another bug.

I haven't looked into the details of this, but agree about raising a
thread on -hackers about it.

Thanks,
Amit

  5. Re: BUG #15623: Inconsistent use of default for updatable view

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2019-02-10T00:48:12Z

    On Sat, 9 Feb 2019 at 16:57, Amit Langote <amitlangote09@gmail.com> wrote:
> On Fri, Feb 8, 2019 at 8:01 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> > On Fri, 8 Feb 2019 at 05:07, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> > > Looking into this, the reason it works when inserting just one row vs.
> > > more than one row is that those two cases are handled by nearby but
> > > different pieces of code.  The code that handles multiple rows seems buggy
> > > as seen in the above example.  Specifically, I think the bug is in
> > > rewriteValuesRTE() which is a function to replace the default placeholders
> > > in the input rows by the default values as defined for the target
> > > relation.  It is called twice when inserting via the view -- first for the
> > > view relation and then again for the underlying table.
> >
> > Right, except when the view is trigger-updatable...
>
> Oops, I missed this bit.  Updated the patch per your suggestion and
> expanded the test case to exercise this.
>

Unfortunately, that's still not quite right because it makes the
behaviour of single- and multi-row inserts inconsistent for
rule-updatable views. Attached is an updated patch that fixes that. I
adjusted the tests a bit to try to make it clearer which defaults get
applied, and test all possibilities.

However, this is still not the end of the story, because it doesn't
fix the fact that, if the view has a DO ALSO rule on it, single-row
inserts behave differently from multi-row inserts. In that case, each
insert becomes 2 inserts, and defaults need to be treated differently
in each of the 2 queries. That's going to need a little more thought.

Regards,
Dean

  6. Re: BUG #15623: Inconsistent use of default for updatable view

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2019-02-10T11:18:46Z

    On Sun, 10 Feb 2019 at 00:48, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> However, this is still not the end of the story, because it doesn't
> fix the fact that, if the view has a DO ALSO rule on it, single-row
> inserts behave differently from multi-row inserts. In that case, each
> insert becomes 2 inserts, and defaults need to be treated differently
> in each of the 2 queries. That's going to need a little more thought.
>

Here's an updated patch to handle that case.

In case it's not obvious, I'm not intending to try to get this into
next week's updates -- more time is needed to be sure of this fix, and
more pairs of eyes would definitely be helpful, once those updates
have been shipped.

Regards,
Dean

  7. Re: BUG #15623: Inconsistent use of default for updatable view

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2019-02-12T10:33:33Z

    On Sun, 10 Feb 2019 at 11:18, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Sun, 10 Feb 2019 at 00:48, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> > However, this is still not the end of the story, because it doesn't
> > fix the fact that, if the view has a DO ALSO rule on it, single-row
> > inserts behave differently from multi-row inserts. In that case, each
> > insert becomes 2 inserts, and defaults need to be treated differently
> > in each of the 2 queries. That's going to need a little more thought.
> >
>
> Here's an updated patch to handle that case.
>
> In case it's not obvious, I'm not intending to try to get this into
> next week's updates -- more time is needed to be sure of this fix.

So I did some more testing of this and I'm reasonably happy that this
now fixes the originally reported issue of inconsistent handling of
DEFAULTS in multi-row VALUES lists vs single-row ones. I tested
various other scenarios involving conditional/unconditional
also/instead rules, and I didn't find any other surprises. Attached is
an updated patch with improved comments, and a little less code
duplication.

Regards,
Dean

  8. Re: BUG #15623: Inconsistent use of default for updatable view

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-02-13T03:02:25Z

    Hi Dean,

On 2019/02/12 19:33, Dean Rasheed wrote:
> On Sun, 10 Feb 2019 at 11:18, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>> On Sun, 10 Feb 2019 at 00:48, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>>> However, this is still not the end of the story, because it doesn't
>>> fix the fact that, if the view has a DO ALSO rule on it, single-row
>>> inserts behave differently from multi-row inserts. In that case, each
>>> insert becomes 2 inserts, and defaults need to be treated differently
>>> in each of the 2 queries. That's going to need a little more thought.
>>>
>>
>> Here's an updated patch to handle that case.
>>
>> In case it's not obvious, I'm not intending to try to get this into
>> next week's updates -- more time is needed to be sure of this fix.
> 
> So I did some more testing of this and I'm reasonably happy that this
> now fixes the originally reported issue of inconsistent handling of
> DEFAULTS in multi-row VALUES lists vs single-row ones. I tested
> various other scenarios involving conditional/unconditional
> also/instead rules, and I didn't find any other surprises. Attached is
> an updated patch with improved comments, and a little less code
> duplication.

Thanks for updating the patch.

I can't really comment on all of the changes that that you made
considering various cases, but became curious if the single-row and
multi-row inserts cases could share the code that determines if the
DEFAULT item be replaced by the target-relation-specified default or NULL?
 IOW, is there some reason why we can't avoid the special handling for the
multi-row (RTE_VALUES) case?

Thanks,
Amit

  9. Re: BUG #15623: Inconsistent use of default for updatable view

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2019-02-13T09:04:40Z

    On Wed, 13 Feb 2019 at 03:02, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>
> I can't really comment on all of the changes that that you made
> considering various cases, but became curious if the single-row and
> multi-row inserts cases could share the code that determines if the
> DEFAULT item be replaced by the target-relation-specified default or NULL?
>  IOW, is there some reason why we can't avoid the special handling for the
> multi-row (RTE_VALUES) case?
>

No, not as far as I can see.

The tricky part for a multi-row insert is working out what to do when
it sees a DEFAULT, and there is no column default on the target
relation. For an auto-updatable view, it needs to leave the DEFAULT
untouched, so that it can later apply the base relation's column
default when it recurses. For all other kinds of relation, it needs to
turn the DEFAULT into a NULL.

For a single-row insert, that's all much easier. If it sees a DEFAULT,
and there is no column default, it simply omits that entry from the
targetlist. If it then recurses to the base relation, it will put the
targetlist entry back in, if the base relation has a column default.
So it doesn't need to know locally whether it's an auto-updatable
view, and the logic is much simpler. The multi-row case can't easily
do that (add and remove columns) because it's working with a
fixed-width table structure.

Actually, that's not quite the end of it. So far, this has only been
considering INSERT's. I think there are more issues with UPDATE's, but
that's a whole other can of worms. I think I'll commit this first, and
start a thread on -hackers to discuss that.

Regards,
Dean

  10. Re: BUG #15623: Inconsistent use of default for updatable view

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-02-13T09:33:05Z

    On 2019/02/13 18:04, Dean Rasheed wrote:
> On Wed, 13 Feb 2019 at 03:02, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>
>> I can't really comment on all of the changes that that you made
>> considering various cases, but became curious if the single-row and
>> multi-row inserts cases could share the code that determines if the
>> DEFAULT item be replaced by the target-relation-specified default or NULL?
>>  IOW, is there some reason why we can't avoid the special handling for the
>> multi-row (RTE_VALUES) case?
>>
> 
> No, not as far as I can see.
> 
> The tricky part for a multi-row insert is working out what to do when
> it sees a DEFAULT, and there is no column default on the target
> relation. For an auto-updatable view, it needs to leave the DEFAULT
> untouched, so that it can later apply the base relation's column
> default when it recurses. For all other kinds of relation, it needs to
> turn the DEFAULT into a NULL.
> 
> For a single-row insert, that's all much easier. If it sees a DEFAULT,
> and there is no column default, it simply omits that entry from the
> targetlist. If it then recurses to the base relation, it will put the
> targetlist entry back in, if the base relation has a column default.
> So it doesn't need to know locally whether it's an auto-updatable
> view, and the logic is much simpler. The multi-row case can't easily
> do that (add and remove columns) because it's working with a
> fixed-width table structure.

Hmm yeah, column sets must be the same in in all value-lists.

> Actually, that's not quite the end of it. So far, this has only been
> considering INSERT's. I think there are more issues with UPDATE's, but
> that's a whole other can of worms. I think I'll commit this first, and
> start a thread on -hackers to discuss that.

Sure, thanks for the explanation.

Regards,
Amit

  11. Re: BUG #15623: Inconsistent use of default for updatable view

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2019-02-27T09:37:11Z

    On Tue, 12 Feb 2019 at 10:33, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> Here's an updated patch ...

So I pushed that. However, ...

Playing around with it some more, I realised that whilst this appeared
to fix the reported problem, it exposes another issue which is down to
the interaction between rewriteTargetListIU() and rewriteValuesRTE()
--- for an INSERT with a VALUES RTE, rewriteTargetListIU() computes a
list of target attribute numbers (attrno_list) from the targetlist in
its original order, which rewriteValuesRTE() then relies on being the
same length (and in the same order) as each of the lists in the VALUES
RTE. That's OK for the initial invocation of those functions, but it
breaks down when they're recursively invoked for auto-updatable views.

For example, the following test-case, based on a slight variation of
the new regression tests:

create table foo (a int default 1, b int);
create view foo_v as select * from foo;
alter view foo_v alter column b set default 2;
insert into foo_v values (default), (default);

triggers the following Assert in rewriteValuesRTE():

    /* Check list lengths (we can assume all the VALUES sublists are alike) */
    Assert(list_length(attrnos) == list_length(linitial(rte->values_lists)));

What's happening is that the initial targetlist, which contains just 1
entry for the column a, gains another entry to set the default for
column b from the view. Then, when it recurses into
rewriteTargetListIU()/rewriteValuesRTE() for the base relation, the
size of the targetlist (now 2) no longer matches the sizes of the
VALUES RTE lists (1).

I think that that failed Assert was unreachable prior to 41531e42d3,
because the old version of rewriteValuesRTE() would always replace all
unresolved DEFAULT items with NULLs, so when it recursed into
rewriteValuesRTE() for the base relation, it would always bail out
early because there would be no DEFAULT items left, and so it would
fail to notice the list size mismatch.

My first thought was that this could be fixed by having
rewriteTargetListIU() compute attrno_list using only those targetlist
entries that refer to the VALUES RTE, and thus omit any new entries
added due to view defaults. That doesn't work though, because that's
not the only way that a list size mismatch can be triggered --- it's
also possible that rewriteTargetListIU() will merge together
targetlist entries, for example if they're array element assignments
referring to the same column, in which case the rewritten targetlist
could be shorter than the VALUES RTE lists, and attempting to compute
attrno_list from it correctly would be trickier.

So actually, I think the right way to fix this is to give up trying to
compute attrno_list in rewriteTargetListIU(), and have
rewriteValuesRTE() work out the attribute assignments itself for each
column of the VALUES RTE by examining the rewritten targetlist. That
looks to be quite straightforward, and results in a cleaner separation
of logic between the 2 functions, per the attached patch.

I think that rewriteValuesRTE() should only ever see DEFAULT items for
columns that are simple assignments to columns of the target relation,
so it only needs to work out the target attribute numbers for TLEs
that contain simple Vars referring to the VALUES RTE. Any DEFAULT seen
in a column not matching that would be an error, but actually I think
such a thing ought to be a "can't happen" error because of the prior
checks during parse analysis, so I've used elog() to report if this
does happen.

Incidentally, it looks like the part of rewriteValuesRTE()'s comment
about subscripted and field assignment has been wrong since
a3c7a993d5, so I've attempted to clarify that. That will need to look
different pre-9.6, I think.

Thoughts?

Regards,
Dean

  12. Re: BUG #15623: Inconsistent use of default for updatable view

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-02-28T07:46:57Z

    On 2019/02/27 18:37, Dean Rasheed wrote:
> On Tue, 12 Feb 2019 at 10:33, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>> Here's an updated patch ...
> 
> So I pushed that. However, ...
> 
> Playing around with it some more, I realised that whilst this appeared
> to fix the reported problem, it exposes another issue which is down to
> the interaction between rewriteTargetListIU() and rewriteValuesRTE()
> --- for an INSERT with a VALUES RTE, rewriteTargetListIU() computes a
> list of target attribute numbers (attrno_list) from the targetlist in
> its original order, which rewriteValuesRTE() then relies on being the
> same length (and in the same order) as each of the lists in the VALUES
> RTE. That's OK for the initial invocation of those functions, but it
> breaks down when they're recursively invoked for auto-updatable views.
>> So actually, I think the right way to fix this is to give up trying to
> compute attrno_list in rewriteTargetListIU(), and have
> rewriteValuesRTE() work out the attribute assignments itself for each
> column of the VALUES RTE by examining the rewritten targetlist. That
> looks to be quite straightforward, and results in a cleaner separation
> of logic between the 2 functions, per the attached patch.

+1.  Only rewriteValuesRTE needs to use that information, so it's better
to teach it to figure it by itself.

> I think that rewriteValuesRTE() should only ever see DEFAULT items for
> columns that are simple assignments to columns of the target relation,
> so it only needs to work out the target attribute numbers for TLEs
> that contain simple Vars referring to the VALUES RTE. Any DEFAULT seen
> in a column not matching that would be an error, but actually I think
> such a thing ought to be a "can't happen" error because of the prior
> checks during parse analysis, so I've used elog() to report if this
> does happen.

+                if (attrno == 0)
+                    elog(ERROR, "Cannot set value in column %d to
DEFAULT", i);

Maybe: s/Cannot/cannot/g

+        Assert(list_length(sublist) == numattrs);

Isn't this Assert useless, because we're setting numattrs to
list_length(<first-sublist>) and transformInsertStmt ensures that all
sublists are same length?


Thanks,
Amit

  13. Re: BUG #15623: Inconsistent use of default for updatable view

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2019-02-28T14:07:46Z

    On Thu, 28 Feb 2019 at 07:47, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>
> +                if (attrno == 0)
> +                    elog(ERROR, "Cannot set value in column %d to
> DEFAULT", i);
>
> Maybe: s/Cannot/cannot/g
>

Ah yes, you're right. That is the convention.


> +        Assert(list_length(sublist) == numattrs);
>
> Isn't this Assert useless, because we're setting numattrs to
> list_length(<first-sublist>) and transformInsertStmt ensures that all
> sublists are same length?
>

Well possibly I'm being over-paranoid, but given that it may have come
via a previous invocation of rewriteValuesRTE() that may have
completely rebuilt the lists, it seemed best to be sure that it hadn't
done something unexpected. It's about to use that to read from the
attrnos array, so it might read beyond the array bounds if any of the
prior logic was faulty.

Thanks for looking.

Regards,
Dean