`yamltodb` does not quote all keyword identifiers

[rmg]

https://github.com/perseas/Pyrseas/blob/171d703ce52bd61055058ed48dca3c62654b5f17/pyrseas/dbobject/__init__.py#L37-L39

The identifier quoting is currently only done for what Pyrseas thinks are reserved words. Unfortunately, that criteria misses some other reserved keywords:

SELECT word FROM pg_get_keywords() WHERE catdesc like 'reserved%' and catcode != 'R';
      word
----------------
 authorization
 binary
 collation
 concurrently
 cross
 current_schema
 freeze
 full
 ilike
 inner
 is
 isnull
 join
 left
 like
 natural
 notnull
 outer
 overlaps
 right
 similar
 tablesample
 verbose
(23 rows)

In my experience, query builders (as found in things like django, Ruby on Rails, etc.) generally unconditionally quote all identifiers.

https://github.com/perseas/Pyrseas/blob/171d703ce52bd61055058ed48dca3c62654b5f17/pyrseas/dbobject/__init__.py#L58

I can certainly see how it could be a breaking change to switch to it, but that aside it seems like a much safer approach. Is there a reason this is only done conditionally in Pyrseas that I'm missing?

Even if the quoting is left as conditional, the reserved words list is definitely incomplete.

Originally posted by @rmg in https://github.com/perseas/Pyrseas/issues/177#issuecomment-561862459

[rmg]

Looking through the older issues, it seems this has come up a few times before.

@jmafc said 6 years ago in https://github.com/perseas/Pyrseas/issues/69#issuecomment-27255728:

It had been suggested in the mailing list that we quote everything but IIRC there were some ambiguous situations with that approach.

[rmg]

The particular collision that triggered this was a ~table~ column named authorization, which would be handled by an expanded RESERVED_WORDS query.

[jmafc]

I think that in order to be safe we'll have to change the query to:

SELECT word FROM pg_get_keywords() 
WHERE catcode != 'U';

The classification system looks very weird: 'table' is reserved, but 'view' and 'type' are unreserved, 'values' is unreserved but it cannot be a function or type name (which means it is reserved for those uses).

[rmg]

Is there a risk with going even broader since the reserved word check is only considering exact matches anyway?

SELECT word FROM pg_get_keywords();

[jmafc]

It's a trade-off. Without a WHERE clause you currently get 440 words, with my suggested restriction only 150. The extra 290 are words that Postgres considers "unreserved" with no ifs or buts. If they become reserved (or partially reserved) at a later date, pg_get_keywords() should be updated accordingly.

[rmg]

I'm in the "quote all the things" camp, so really I'm just trying to get as close to that as is acceptable ;-)

Looking through the test suite now to see just how tedious it would be to actually implement that.. so many strings 😵

[rmg]

@jmafc any chance of cutting a new release?

[jmafc]

On the one hand, not much has happened here since the 0.9.0 release last July, so I guess we could release 0.9.1. On the other, since PG 12 has been released, I think 0.9.1 should only be released if there are no regressions or problems when run against PG 12. You did make it so Travis runs PG 12 too, but wasn't there some issues with that?

[rmg]

I'll take a look on Monday.

[jmafc]

@rmg Whatever happened to this?

[rmg]

Sorry, got stuck bouncing between work and home computers and my follow-up fell through the cracks.

When I took a look at the state of the test suite it looked to me like >=12 had fewer exceptions/deviations than <10, so I think support for v12 is probably decent.

The only changes that had to be made to get the test suite passing with pg 12 were:

• f0f6181 - testing: replace plpythonu with plpython3u
• d54b5e2 - pg12: expect collation on name type columns
• 3b842f9 - pg12: expect longer constraint names

I'm no expert, but they seem pretty benign to me.

[jmafc]

Ok, I'll try to get a release prepared next week.

[jmafc]

Aside from everything else going on, I've had some other issues so I haven't made the time to get the release out. I'll see if I can do that this coming week.