BUG [postgresql]: `CREATE FUNCTION` throws a parsing error when the return type is schema-qualified

[joelmukuthu]

The following throws a parse error, while RETURNS customer works as expected.

CREATE FUNCTION public.get_customers()
RETURNS public.customer
LANGUAGE SQL
AS $BODY$
  SELECT * FROM customer;
$BODY$;

I understand that postgres support is experimental, but I opened a bug report since #63 documents that RETURN type should be supported.

Thanks a lot for your work on this library and on the prettier plugin as well! <3

[nene]

Thanks for reporting.

It's partly a known issue as currently there's no support for schema-qualified types.

Though this particular example now shows me that in case of PostgreSQL, it's actually not possible to properly distinguish between types and other identifiers at syntax level. I was planning to implement an sqlTypeCase: upper/lower option in the prettier plugin. Because PostgreSQL types can be schema-qualified it would already be problematic to have them formatted as schema.MY_TYPE. But when tables can be used as types, then one really can't format types in different case than other identifiers as it would be really bad to have the name of table formatted differently in different contexts. Like CREATE TABLE customer v/s RETURNS CUSTOMER.

[joelmukuthu]

Thanks for responding.

Is there a way to tell the prettier plugin to ignore this parse error? I have sqlAcceptUnsupportedGrammar: true in my config but it doesn't seem to skip over this. I'd be interested in creating a PR for this, is there a manageable amount of work to it?

Regarding the sqlTypeCase option, it sounds like it would be a useful addition but yeah, I'm not sure there's a way to distinguish them at syntax level. Although it could be supported for builtin types only (maybe named sqlBuiltinTypeCase to be explicit) but even then I guess it'd require a hard-coded list of type-names, which you may not be willing to maintain. For me personally, it'd handle all my use-cases (off the top of my head) if it supported built-ins only.

[nene]

Unfortunately that sqlAcceptUnsupportedGrammar only works when a full statement is unsupported. Like if you're using CREATE SERVER which is completely unsupported.

I'd be interested in creating a PR for this, is there a manageable amount of work to it?

That wouldn't really be a good issue to work on, as a) I'm not even sure by myself how to best implement it, b) it might involve a major reorganization of how types are parsed.

Like, if you'd like to help with PostgreSQL support, it would be simpler to add support for a whole new statement (e.g. CREATE SERVER), for which one can just follow the example set by existing code.

[nene]

The good news though is that dealing with this problem is kinda at the top of my priorities list, as I'm working with a PostgreSQL project during my day job, and the missing support for schema-qualified types is currently blocking me from implementing some code-generation I'd like to have.

[nene]

Although it could be supported for builtin types only (maybe named sqlBuiltinTypeCase to be explicit) but even then I guess it'd require a hard-coded list of type-names, which you may not be willing to maintain.

Maintaining list of builtin types is not really an issue, as I'm already doing that :) Plus, several builtin types have special syntax, like CHARACTER VARYING (255).

[joelmukuthu]

It's great to hear that this is on your radar! It's currently the only blocker preventing me from using the prettier plugin in a project. On that note, I've also noticed that RETURNS SETOF type is also failing. Or is that currently unsupported?

Wrt contributing, currently I can only help out with things that are blockers for me, otherwise time won't allow. But when that changes I'll get back in touch :) Is RETURNS SETOF something that I could help out with?

Unfortunately that sqlAcceptUnsupportedGrammar only works when a full statement is unsupported.

Understood!

Maintaining list of builtin types is not really an issue

Also good to know!

Btw, wrt parsing schema-qualified types, would it be possible to update the parser so it treats them as unqualified (maybe via an undocumented option for prettier-plugin-sql-cst)? The CST would definitely be wrong, but it would allow the plugin to format the file in the meantime. I know it's a hack and I wouldn't even do it myself, I just wanted to throw it out there and see what you think :)

[nene]

RETURNS SETOF

I guess that falls into the buck of stuff that's missing from the types support.

Is RETURNS SETOF something that I could help out with?

Probably not before you're somewhat familiar with the codebase. Like for start, are you familiar with Peggy syntax for writing grammars?

Btw, wrt parsing schema-qualified types, would it be possible to update the parser so it treats them as unqualified

Doesn't really make sense. The parser first needs to recognize that syntax, after that it can treat it as something different.

[joelmukuthu]

Are you familiar with Peggy syntax for writing grammars?

Not at all, so I think I can't be of much help there. All the same I'm here if there's anything else I can help out with!

[nene]

So, I managed to implement parsing of schema-qualified types for PostgreSQL.

I opted to parse all PostgreSQL type names as identifiers (e.g. int), or list of identifiers (e.g. character varying).

There's still one bit about which I'm not really sure: TIMESTAMP WITH TIME ZONE. Currently this gets parsed so that timestamp is parsed as identifier and WITH TIME ZONE is parsed as list of keywords. I'll probably just handle casing of this on the prettier side. I'm kind of one the fence about this. On one hand timestamp WITH TIME ZONE looks wrong. Though if it's written as timestamp (3) WITH TIME ZONE it looks more OK, but I think very few people use the precision parameter.

On the prettier-plugin side that will mean that sqlKeywordCase option will no more apply for Postgres type names. But as the case of identifiers is currently preserved as-is, that would mean re-running prettier over already formatted SQL would cause no changes. At least not until I implement sqlDataTypeCase or sqlIdentifierCase options.