Re: Add Boolean node

Hi,

On 2021-12-27 10:02:14 +0100, Peter Eisentraut wrote:
> This patch adds a new node type Boolean, to go alongside the "value" nodes
> Integer, Float, String, etc. This seems appropriate given that Boolean
> values are a fundamental part of the system and are used a lot.
>
> Before, SQL-level Boolean constants were represented by a string with
> a cast, and internal Boolean values in DDL commands were usually represented
> by Integer nodes. This takes the place of both of these uses, making the
> intent clearer and having some amount of type safety.

This annoyed me plenty of times before, plus many.

> From 4e1ef56b5443fa11d981eb6e407dfc7c244dc60e Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter(at)eisentraut(dot)org>
> Date: Mon, 27 Dec 2021 09:52:05 +0100
> Subject: [PATCH v1] Add Boolean node
>
> Before, SQL-level boolean constants were represented by a string with
> a cast, and internal Boolean values in DDL commands were usually
> represented by Integer nodes. This takes the place of both of these
> uses, making the intent clearer and having some amount of type safety.
> ---
> ...
> 20 files changed, 210 insertions(+), 126 deletions(-)

This might be easier to review if there were one patch adding the Boolean
type, and then a separate one converting users?

> diff --git a/src/backend/commands/tsearchcmds.c b/src/backend/commands/tsearchcmds.c
> index c47a05d10d..b7261a88d4 100644
> --- a/src/backend/commands/tsearchcmds.c
> +++ b/src/backend/commands/tsearchcmds.c
> @@ -1742,6 +1742,15 @@ buildDefItem(const char *name, const char *val, bool was_quoted)
> return makeDefElem(pstrdup(name),
> (Node *) makeFloat(pstrdup(val)),
> -1);
> +
> + if (strcmp(val, "true") == 0)
> + return makeDefElem(pstrdup(name),
> + (Node *) makeBoolean(true),
> + -1);
> + if (strcmp(val, "false") == 0)
> + return makeDefElem(pstrdup(name),
> + (Node *) makeBoolean(false),
> + -1);
> }
> /* Just make it a string */
> return makeDefElem(pstrdup(name),

Hm. defGetBoolean() interprets "true", "false", "on", "off" as booleans. ISTM
we shouldn't invent different behaviours for individual subsystems?

> --- a/src/backend/nodes/outfuncs.c
> +++ b/src/backend/nodes/outfuncs.c
> @@ -3433,6 +3433,12 @@ _outFloat(StringInfo str, const Float *node)
> appendStringInfoString(str, node->val);
> }
>
> +static void
> +_outBoolean(StringInfo str, const Boolean *node)
> +{
> + appendStringInfoString(str, node->val ? "true" : "false");
> +}

Any reason not to use 't' and 'f' instead? It seems unnecessary to bloat the
node output by the longer strings, and it makes parsing more expensive
too:

> --- a/src/backend/nodes/read.c
> +++ b/src/backend/nodes/read.c
> @@ -283,6 +283,8 @@ nodeTokenType(const char *token, int length)
> retval = RIGHT_PAREN;
> else if (*token == '{')
> retval = LEFT_BRACE;
> + else if (strcmp(token, "true") == 0 || strcmp(token, "false") == 0)
> + retval = T_Boolean;
> else if (*token == '"' && length > 1 && token[length - 1] == '"')
> retval = T_String;
> else if (*token == 'b')

Before this could be implemented as a jump table, not now it can't easily be
anymore.

> diff --git a/src/test/isolation/expected/ri-trigger.out b/src/test/isolation/expected/ri-trigger.out
> index 842df80a90..db85618bef 100644
> --- a/src/test/isolation/expected/ri-trigger.out
> +++ b/src/test/isolation/expected/ri-trigger.out
> @@ -4,9 +4,9 @@ starting permutation: wxry1 c1 r2 wyrx2 c2
> step wxry1: INSERT INTO child (parent_id) VALUES (0);
> step c1: COMMIT;
> step r2: SELECT TRUE;
> -bool
> -----
> -t
> +?column?
> +--------
> +t
> (1 row)

This doesn't seem great. It might be more consistent ("SELECT 1" doesn't end
up with 'integer' as column name), but this still seems like an unnecessarily
large user-visible change for an internal data-representation change?

Greetings,

Andres Freund