Re: Boolean partitions syntax

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Stephen Frost <sfrost(at)snowman(dot)net>, Mark Dilger <hornschnorter(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Boolean partitions syntax
Date: 2018-01-29 04:21:54
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2018/01/27 1:31, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Fri, Jan 26, 2018 at 11:07 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>>> I've already had two people mention that it'd be neat to have PG support
>>> it, so I don't believe it'd go unused. As for if we should force people
>>> to use quotes, my vote would be no because we don't require that for
>>> other usage of true/false in the parser and I don't see any reason why
>>> this should be different.
>> OK. Let's wait a bit and see if anyone else wants to weigh in.
> I dunno, this patch seems quite bizarre to me. IIUC, it results in
> TRUE/FALSE behaving differently in a partbound_datum than they do
> anywhere else in the grammar, to wit that they're effectively just
> another spelling for the undecorated literals 't' and 'f', without
> anything indicating that they're boolean. That seems wrong from a
> data typing standpoint. And even if it's really true that we'd
> rather lose type information for partbound literals, a naive observer
> would probably think that these should decay to the strings "true"
> and "false" not "t" and "f" (cf. opt_boolean_or_string).

Partition bound literals as captured gram.y don't have any type
information attached. They're carried over in a A_Const to
transformPartitionBoundValue() and coerced to the target partition key
type there. Note that each of the the partition bound literal datums
received from gram.y is castNode(A_Const)'d in parse_utilcmds.c.

I agree that it's better to simply makeStringConst("true"/"false") for
TRUE_P and FALSE_P, instead of makingBoolAConst(true/false).

> I've not paid any attention to this thread up to now, so maybe there's
> a rational explanation for this choice that I missed. But mucking
> with makeBoolAConst like that doesn't seem to me to pass the smell
> test. I'm on board with the stated goal of allowing TRUE/FALSE here,
> but having to contort the grammar output like this suggests that
> there's something wrong in parse analysis of partbound_datum.

Attached updated patch doesn't change anything about makeBoolAConst and
now is just a 2-line change to gram.y.

> PS: the proposed documentation wording is too verbose by half.
> I'd just cut it down to "<literal constant>".

Yeah, I was getting nervous about the lines in syntax synopsis getting
unwieldily long after this change. I changed all of them to use
literal_constant for anything other than special keywords MINVALUE and
MAXVALUE and a paragraph in the description to clarify.

Attached updated patch. Thanks for the comments.


Attachment Content-Type Size
v4-0001-Allow-Boolean-values-in-partition-FOR-VALUES-clau.patch text/plain 4.9 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Jing Wang 2018-01-29 04:29:35 Re: [HACKERS] WIP: Separate log file for extension
Previous Message Tom Lane 2018-01-29 04:07:26 Re: Redefining inet_net_ntop