That looks like a good change. I wonder what motivates that now? Why wasn't it added when the usages grew? Are there more Boolean usages planned?
I ask because this code change will affect ability to automatically cherry-pick some of the patches. defGetBoolean() - please update the comment in the default to case to mention defGetString along with opt_boolean_or_string production. Reading the existing code in that function, one would wonder why to use true and false over say on and off. But true/false seems a natural choice. So that's fine. defGetBoolean() and nodeRead() could use a common function to parse a boolean string. The code in nodeRead() seems to assume that any string starting with "t" will represent value true. Is that right? We are using literal constants "true"/"false" at many places. This patch adds another one. I am wondering whether it makes sense to add #define TRUE_STR, FALSE_STR and use it everywhere for consistency and correctness. For the sake of consistency (again :)), we should have a function to return string representation of a Boolean node and use it in both defGetString and _outBoolean(). Are the expected output changes like below necessary? Might affect backward compatibility for applications. -bool ----- -t +?column? +-------- +t On Mon, Dec 27, 2021 at 2:32 PM Peter Eisentraut <peter.eisentr...@enterprisedb.com> 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. -- Best Wishes, Ashutosh Bapat
