On Wed, Oct 19, 2022 at 03:45:48PM +0900, Michael Paquier wrote: > With this in mind, would somebody complain if I commit that? That's a > nice reduction in code, while completing the work done in 40c24bf: > 25 files changed, 338 insertions(+), 477 deletions(-)
On second look, there is something I have underestimated here with FigureColnameInternal(). This function would create an attribute name based on the SQL keyword given in input. For example, on HEAD we would get that: =# SELECT * FROM CURRENT_CATALOG; current_catalog ----------------- postgres (1 row) But the patch enforces the attribute name to be the underlying function name, switching the previous "current_catalog" to "current_database". For example: =# SELECT * FROM CURRENT_CATALOG; current_database ------------------ postgres (1 row) I am not sure how much it matters in practice, but this could break some queries. One way to tackle that is to extend FigureColnameInternal() so as we use a compatible name when the node is a T_FuncCall, but that won't be entirely water-proof as long as there is not a one-one mapping between the SQL keywords and the underlying function names, aka we would need a current_catalog. "user" would be also too generic as a catalog function name, so we should name its proc entry to a pg_user anyway, requiring a shortcut in FigureColnameInternal(). Or perhaps I am worrying too much and keeping the code simpler is better? Does the SQL specification require that the attribute name has to match its SQL keyword when specified in a FROM clause when there is no aliases? Thoughts? -- Michael
signature.asc
Description: PGP signature
