On 27/08/2018 11:17, Fabien COELHO wrote:
> About patch 3: applies cleanly independently of the 2 others, compiles,
> "make check" is okay.
>
> A few comments:
>
> There seems to be several somehow unrelated changes: one about copy,
> one about trigger and one about constraints? The two later changes do not
> seem to impact the tests, though.
added more tests
> In "CreateTrigger", you moved "make_parsestate" but removed
> "free_parsestate". I'd rather move it than remove it.
See also previous discussion, but I've moved it around for now.
> In "value.h", the added location field deserves a "/* token location, or
> -1 if unknown */" comment like others in "parsenode.h", "plannode.h" and
> "primnodes.h".
done
> Copying and comparing values are updaed, but value in/out functions are
> not updated to read/write the location, although other objects have their
> location serialized. ISTM that it should be done as well.
Hmm, maybe that's a problem, because the serialization of a Value node
is just a scalar. It doesn't have any structure where to put additional
fields. Maybe we should think about not using Value as a parse
representation for column name lists. Let me think about that.
Attached is another patch set. I think the first two patches are OK
now, but the third one might need to be rethought.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
