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@eisentraut.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