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

Thanks for looking at this.

On Fri, Feb 8, 2019 at 8:01 PM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>
> On Fri, 8 Feb 2019 at 05:07, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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