Horton

Thread

Commits

  1. Fix omission of locations in outfuncs/readfuncs partitioning node support.

  2. Code review focused on new node types added by partitioning support.

  1. pgsql: Code review focused on new node types added by partitioning supp

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-29T03:20:41Z

    Code review focused on new node types added by partitioning support.

Fix failure to check that we got a plain Const from const-simplification of
a coercion request.  This is the cause of bug #14666 from Tian Bing: there
is an int4 to money cast, but it's only stable not immutable (because of
dependence on lc_monetary), resulting in a FuncExpr that the code was
miserably unequipped to deal with, or indeed even to notice that it was
failing to deal with.  Add test cases around this coercion behavior.

In view of the above, sprinkle the code liberally with castNode() macros,
in hope of catching the next such bug a bit sooner.  Also, change some
functions that were randomly declared to take Node* to take more specific
pointer types.  And change some struct fields that were declared Node*
but could be given more specific types, allowing removal of assorted
explicit casts.

Place PARTITION_MAX_KEYS check a bit closer to the code it's protecting.
Likewise check only-one-key-for-list-partitioning restriction in a less
random place.

Avoid not-per-project-style usages like !strcmp(...).

Fix assorted failures to avoid scribbling on the input of parse
transformation.  I'm not sure how necessary this is, but it's entirely
silly for these functions to be expending cycles to avoid that and not
getting it right.

Add guards against partitioning on system columns.

Put backend/nodes/ support code into an order that matches handling
of these node types elsewhere.

Annotate the fact that somebody added location fields to PartitionBoundSpec
and PartitionRangeDatum but forgot to handle them in
outfuncs.c/readfuncs.c.  This is fairly harmless for production purposes
(since readfuncs.c would just substitute -1 anyway) but it's still bogus.
It's not worth forcing a post-beta1 initdb just to fix this, but if we
have another reason to force initdb before 10.0, we should go back and
clean this up.

Contrariwise, somebody added location fields to PartitionElem and
PartitionSpec but forgot to teach exprLocation() about them.

Consolidate duplicative code in transformPartitionBound().

Improve a couple of error messages.

Improve assorted commentary.

Re-pgindent the files touched by this patch; this affects a few comment
blocks that must have been added quite recently.

Report: https://postgr.es/m/20170524024550.29935.14396@wrigleys.postgresql.org

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/76a3df6e5e621928fbf0cddf347e16a62e9433ec

Modified Files
--------------
src/backend/catalog/heap.c                 |   2 +-
src/backend/catalog/partition.c            |  64 +++++----
src/backend/commands/tablecmds.c           |  90 ++++++++----
src/backend/nodes/copyfuncs.c              |  30 ++--
src/backend/nodes/equalfuncs.c             |  22 +--
src/backend/nodes/nodeFuncs.c              |   6 +
src/backend/nodes/outfuncs.c               |  28 ++--
src/backend/nodes/readfuncs.c              |   4 +
src/backend/parser/gram.y                  |  29 ++--
src/backend/parser/parse_utilcmd.c         | 223 ++++++++++++++---------------
src/backend/utils/adt/ruleutils.c          |  28 ++--
src/include/catalog/heap.h                 |   3 +-
src/include/catalog/partition.h            |   6 +-
src/include/nodes/nodes.h                  |   2 +-
src/include/nodes/parsenodes.h             |  48 ++++---
src/include/parser/parse_utilcmd.h         |   4 +-
src/test/regress/expected/create_table.out |  25 +++-
src/test/regress/sql/create_table.sql      |  17 +++
18 files changed, 361 insertions(+), 270 deletions(-)

  2. Re: pgsql: Code review focused on new node types added by partitioning supp

    Noah Misch <noah@leadboat.com> — 2017-05-30T02:32:17Z

    On Mon, May 29, 2017 at 03:20:41AM +0000, Tom Lane wrote:
> Annotate the fact that somebody added location fields to PartitionBoundSpec
> and PartitionRangeDatum but forgot to handle them in
> outfuncs.c/readfuncs.c.  This is fairly harmless for production purposes
> (since readfuncs.c would just substitute -1 anyway) but it's still bogus.
> It's not worth forcing a post-beta1 initdb just to fix this, but if we
> have another reason to force initdb before 10.0, we should go back and
> clean this up.

+1 for immediately forcing initdb for this, getting it out of the way.  We're
already unlikely to reach 10.0 without bumping catversion, but if we otherwise
did, releasing 10.0 with a 10beta1 catversion would have negative value.

  3. Re: [COMMITTERS] Re: pgsql: Code review focused on new node types added by partitioning supp

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-30T02:38:08Z

    Noah Misch <noah@leadboat.com> writes:
> On Mon, May 29, 2017 at 03:20:41AM +0000, Tom Lane wrote:
>> Annotate the fact that somebody added location fields to PartitionBoundSpec
>> and PartitionRangeDatum but forgot to handle them in
>> outfuncs.c/readfuncs.c.  This is fairly harmless for production purposes
>> (since readfuncs.c would just substitute -1 anyway) but it's still bogus.
>> It's not worth forcing a post-beta1 initdb just to fix this, but if we
>> have another reason to force initdb before 10.0, we should go back and
>> clean this up.

> +1 for immediately forcing initdb for this, getting it out of the way.  We're
> already unlikely to reach 10.0 without bumping catversion, but if we otherwise
> did, releasing 10.0 with a 10beta1 catversion would have negative value.

I'm not really for doing it that way, but I'm willing to apply the fix
if there's consensus for your position.  Anybody else have an opinion?

			regards, tom lane

  4. Re: [COMMITTERS] Re: pgsql: Code review focused on new node types added by partitioning supp

    Stephen Frost <sfrost@snowman.net> — 2017-05-30T02:41:38Z

    * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Mon, May 29, 2017 at 03:20:41AM +0000, Tom Lane wrote:
> >> Annotate the fact that somebody added location fields to PartitionBoundSpec
> >> and PartitionRangeDatum but forgot to handle them in
> >> outfuncs.c/readfuncs.c.  This is fairly harmless for production purposes
> >> (since readfuncs.c would just substitute -1 anyway) but it's still bogus.
> >> It's not worth forcing a post-beta1 initdb just to fix this, but if we
> >> have another reason to force initdb before 10.0, we should go back and
> >> clean this up.
> 
> > +1 for immediately forcing initdb for this, getting it out of the way.  We're
> > already unlikely to reach 10.0 without bumping catversion, but if we otherwise
> > did, releasing 10.0 with a 10beta1 catversion would have negative value.
> 
> I'm not really for doing it that way, but I'm willing to apply the fix
> if there's consensus for your position.  Anybody else have an opinion?

I tend to agree with Noah on this one.

Thanks!

Stephen

  5. Re: [COMMITTERS] Re: pgsql: Code review focused on new node types added by partitioning supp

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-05-30T03:53:57Z

    On 2017/05/30 11:41, Stephen Frost wrote:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Noah Misch <noah@leadboat.com> writes:
>>> On Mon, May 29, 2017 at 03:20:41AM +0000, Tom Lane wrote:
>>>> Annotate the fact that somebody added location fields to PartitionBoundSpec
>>>> and PartitionRangeDatum but forgot to handle them in
>>>> outfuncs.c/readfuncs.c.  This is fairly harmless for production purposes
>>>> (since readfuncs.c would just substitute -1 anyway) but it's still bogus.
>>>> It's not worth forcing a post-beta1 initdb just to fix this, but if we
>>>> have another reason to force initdb before 10.0, we should go back and
>>>> clean this up.
>>
>>> +1 for immediately forcing initdb for this, getting it out of the way.  We're
>>> already unlikely to reach 10.0 without bumping catversion, but if we otherwise
>>> did, releasing 10.0 with a 10beta1 catversion would have negative value.
>>
>> I'm not really for doing it that way, but I'm willing to apply the fix
>> if there's consensus for your position.  Anybody else have an opinion?
> 
> I tend to agree with Noah on this one.

+1

Thanks,
Amit

  6. Re: [COMMITTERS] Re: pgsql: Code review focused on new node types added by partitioning supp

    Magnus Hagander <magnus@hagander.net> — 2017-05-30T09:26:00Z

    On Tue, May 30, 2017 at 4:41 AM, Stephen Frost <sfrost@snowman.net> wrote:

> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > Noah Misch <noah@leadboat.com> writes:
> > > On Mon, May 29, 2017 at 03:20:41AM +0000, Tom Lane wrote:
> > >> Annotate the fact that somebody added location fields to
> PartitionBoundSpec
> > >> and PartitionRangeDatum but forgot to handle them in
> > >> outfuncs.c/readfuncs.c.  This is fairly harmless for production
> purposes
> > >> (since readfuncs.c would just substitute -1 anyway) but it's still
> bogus.
> > >> It's not worth forcing a post-beta1 initdb just to fix this, but if we
> > >> have another reason to force initdb before 10.0, we should go back and
> > >> clean this up.
> >
> > > +1 for immediately forcing initdb for this, getting it out of the
> way.  We're
> > > already unlikely to reach 10.0 without bumping catversion, but if we
> otherwise
> > > did, releasing 10.0 with a 10beta1 catversion would have negative
> value.
> >
> > I'm not really for doing it that way, but I'm willing to apply the fix
> > if there's consensus for your position.  Anybody else have an opinion?
>
> I tend to agree with Noah on this one.
>
>
>
+1


-- 
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

  7. Re: [COMMITTERS] Re: pgsql: Code review focused on new node types added by partitioning supp

    Robert Haas <robertmhaas@gmail.com> — 2017-05-30T11:50:25Z

    On Tue, May 30, 2017 at 5:26 AM, Magnus Hagander <magnus@hagander.net> wrote:
>> > I'm not really for doing it that way, but I'm willing to apply the fix
>> > if there's consensus for your position.  Anybody else have an opinion?
>>
>> I tend to agree with Noah on this one.
>
> +1

+1 from me, too.  I don't see that there's enough advantage in
avoiding a catversion bump to justify leaving this footgun behind.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  8. Re: [COMMITTERS] Re: pgsql: Code review focused on new node types added by partitioning supp

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-30T13:41:57Z

    Robert Haas <robertmhaas@gmail.com> writes:
>>>> I'm not really for doing it that way, but I'm willing to apply the fix
>>>> if there's consensus for your position.  Anybody else have an opinion?

> +1 from me, too.  I don't see that there's enough advantage in
> avoiding a catversion bump to justify leaving this footgun behind.

The consensus seems pretty clear.  I'll make it so.

			regards, tom lane