Fix: deparse DEFAULT in ColumnDef AT TIME ZONE

[tylergannon]

I submitted a bug report earlier today in the pg_query_go issues, then decided to have a look and see if I could fix it.

This fixes a bug in which DEFAULT expressions are deparsed into invalid SQL.

I added test cases to deparse_tests.c, which are corrected in the suggested fix.

I'm not sure about whether my manner of detecting the situation will suit you, but I hope we can get something going here in the way of a fix. I'm trying to release some software that depends on being able to deparse queries of this shape, and am going to have to, for the time being, devise some kind of workaround in order to ship.

Thanks! Let me know if I can help in any way.

[lelit]

I did not check your implementation, but AFAICT also the following case requires parens:

create table my_table (created_at timestamptz not null default ('1 hour'::interval + current_timestamp at time zone 'UTC'))

so I'm not sure the strategy to look for an immediately preceeding DEFAULT is correct.

[tylergannon]

@lelit good point, thanks for responding. I added that test case and of course it isn't repaired by my attempted fix. What would you suggest?

If it was totally up to me, I'd just parenthesize this type of construct in all cases, in order to ensure that deparse always returns valid SQL, but that is bound to throw off a lot of tests downstream.

Meanwhile, I don't know of there being any contextual info on the StringInfo or FuncCall objects, that would reveal that this is happening inside of a DEFAULT declaration.

Is there any other category I could ascribe to this positioning where the parentheses are required around the expression, which I could use to detect it?

[lelit]

I'm afraid I don't have a quick answer, because I don't know very well the internals of the deparsing tool: as you say there is no (AFAIK) any generic way to pass down what you call contextual info to lower nodes, and that complicates the solution.
One example of a similar problem is in the function deparseTypeCast(), that accepts a context from higher levels to differentiate its output, but in this case it is tricky, because that context (computed in the deparseConstraint() function) should be passed down to a possibly wide range of nodes that may wrap the actual leaf deparseFuncCall() function...
Sorry for not being of a great help, maybe Lukas or msepga may say something more interesting...