perseas / Pyrseas

Provides utilities for Postgres database schema versioning.
https://perseas.github.io/
BSD 3-Clause "New" or "Revised" License
395 stars 67 forks source link

yamltodb fails on Postgres 13 due to removal of pg_pltemplate catalog #226

Open joshcurtis opened 3 years ago

joshcurtis commented 3 years ago

pg_pltemplate was removed in postgres 13 causing yamltodb to fail when this statement runs.

My understanding (based on #39) is that Pyrseas needs a way to distinguish language extensions and other extensions. If so, I think joining pg_language with pg_available_extensions() gives the same result but am not 100% sure.

jmafc commented 3 years ago

The PR didn't work (see Travis logs). I think two things are needed. First, your mod ought to be tested against PG 13. Did you run any of the tests locally, in particular the extensions tests? Anyway we need to change .travis.yml to add that version (we should probably remove versions 9.4 and 9.5 as they're no longer supported). And then we need to investigate the errors and fix them. They all appear to be the same error in test_create_lang_extension in tests/dbobject/test_extension.py.

joshcurtis commented 3 years ago

Thanks for the quick reply!

I didn't run the tests locally, just ran it as part of the script we use. The fix doesn't work at all since the languages don't show up in pg_available_extensions() until CREATE EXTENSION has run.

I'll look again tomorrow and the few other tests that started to fail on 13.

On Wed, Apr 7, 2021, 3:03 PM Joe Abbate @.***> wrote:

The PR didn't work (see Travis logs). I think two things are needed. First, your mod ought to be tested against PG 13. Did you run any of the tests locally, in particular the extensions tests? Anyway we need to change .travis.yml to add that version (we should probably remove versions 9.4 and 9.5 as they're no longer supported). And then we need to investigate the errors and fix them. They all appear to be the same error in test_create_lang_extension in tests/dbobject/test_extension.py.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/perseas/Pyrseas/issues/226#issuecomment-815292858, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABA6HRTA3JSWKYY5VECGT63THTJE5ANCNFSM42RQ4FWA .

alexitheodore commented 2 years ago

I just got really excited about using Pyrseas but the dbtoyaml function erred immediately and I am pretty sure it is this same bug. Its hitting issues with my extensions, which I have several. I dropped one, and it just errors at the next. So yes, I think this is still a bug.

I am probably not suited to help with the python, but I can certainly help with generating pg_* queries to get the right info as per the above? If I can help in that way, give me some specs to work towards.

jmafc commented 2 years ago

@alexitheodore Could you please first show the dbtoyaml error/stack trace? Also, it would help if you could create a simple test case. You can look at pyrseas/tests/dbobject/test_extension.py to see what we're already testing. Later I'll see if I can run those tests on PG 13.

alexitheodore commented 2 years ago

@jmafc Sure, here are two different ones:

First error (seemingly from pg_cron extension)

Traceback (most recent call last):
  File "/usr/local/bin/dbtoyaml", line 11, in <module>
    sys.exit(main())
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbtoyaml.py", line 49, in main
    dbmap = db.to_map()
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/database.py", line 488, in to_map
    dbmap.update(self.db.schemas.to_map(self.db, opts))
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/schema.py", line 357, in to_map
    schemas.update(self[sch].to_map(db, self, opts))
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/schema.py", line 104, in to_map
    schobjs.append((obj, obj.to_map(db, dbschemas, opts)))
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/table.py", line 556, in to_map
    dct['triggers'].update(self.triggers[k.name].to_map(db))
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/trigger.py", line 156, in to_map
    dct = super(Trigger, self).to_map(db)
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/__init__.py", line 361, in to_map
    deps -= self.get_implied_deps(db)
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/trigger.py", line 239, in get_implied_deps
    deps.add(db.functions[fschema, fname, self.arguments or ''])
KeyError: (u'cron', 'job_cache_invalidate', '')

Then I tried dropping that and running again. So the second error looks like it is to do with JSONB - hard to tell if that is extension related or not since it is native...

Traceback (most recent call last):
  File "/usr/local/bin/dbtoyaml", line 11, in <module>
    sys.exit(main())
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbtoyaml.py", line 49, in main
    dbmap = db.to_map()
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/database.py", line 488, in to_map
    dbmap.update(self.db.schemas.to_map(self.db, opts))
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/schema.py", line 357, in to_map
    schemas.update(self[sch].to_map(db, self, opts))
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/schema.py", line 140, in to_map
    schobjs.append((obj, obj.to_map(db, no_owner, no_privs)))
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/function.py", line 569, in to_map
    dct = super(Aggregate, self).to_map(db, no_owner, no_privs)
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/__init__.py", line 361, in to_map
    deps -= self.get_implied_deps(db)
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/function.py", line 666, in get_implied_deps
    deps.add(db.functions[sch, fnc, args])
KeyError: (u'global', 'json_agg_statef', u'jsonb, jsonb')

As for pyrseas/tests/dbobject/test_extension.py - I looked at it, but I don't quite follow Python well enough to create a test case, sorry... I'd be happy to provide SQL tests though?

jmafc commented 2 years ago

Yes, I wasn't expecting you to create a test in test_extension.py, only to get an idea of what we're already testing. You can simply describe the steps of your test case, e.g., createdb testdb, CREATE EXTENSION xyz, etc. Preferably, the extension used in the test should be one generally available such as the pg_trgm we're using.

alexitheodore commented 2 years ago

@jmafc Ok, that I can do.

So, for pg_cron, I did the most remedial test first: fresh cluster/database and adding the pg_cron extension.

Steps to recreate:

Then you will get the error:

Traceback (most recent call last):
  File "/usr/local/bin/dbtoyaml", line 11, in <module>
    sys.exit(main())
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbtoyaml.py", line 49, in main
    dbmap = db.to_map()
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/database.py", line 488, in to_map
    dbmap.update(self.db.schemas.to_map(self.db, opts))
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/schema.py", line 357, in to_map
    schemas.update(self[sch].to_map(db, self, opts))
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/schema.py", line 104, in to_map
    schobjs.append((obj, obj.to_map(db, dbschemas, opts)))
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/table.py", line 556, in to_map
    dct['triggers'].update(self.triggers[k.name].to_map(db))
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/trigger.py", line 156, in to_map
    dct = super(Trigger, self).to_map(db)
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/__init__.py", line 361, in to_map
    deps -= self.get_implied_deps(db)
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/trigger.py", line 239, in get_implied_deps
    deps.add(db.functions[fschema, fname, self.arguments or ''])
KeyError: (u'cron', 'job_cache_invalidate', '')

For what it is worth, job_cache_invalidate is a C language function. I would assume that if the source code is unavailable, it is safe to just skip or put a blank for that.

alexitheodore commented 2 years ago

And here is another test, this time starting from a totally clean slate:

CREATE OR REPLACE FUNCTION json_agg_statef (IN json_cur_in JSONB, IN json_next_in JSONB, OUT json_cur_out JSONB) AS
$$
BEGIN
    json_cur_out := json_cur_in & json_next_in;
END;
$$
LANGUAGE PLPGSQL
IMMUTABLE
PARALLEL SAFE
;

This doesn't cause any errors.

postgres@ip-172-33-2-75:~/adt_tmp$ dbtoyaml -p 5555 postgres
extension plpgsql:
  description: PL/pgSQL procedural language
  owner: postgres
  schema: pg_catalog
  version: '1.0'
schema public:
  description: standard public schema
  function json_agg_statef(json_cur_in jsonb, json_next_in jsonb, OUT json_cur_out jsonb):
    language: plpgsql
    owner: postgres
    returns: jsonb
    source: "\nBEGIN\n    json_cur_out := json_cur_in & json_next_in;\nEND;\n"
    volatility: immutable
  owner: postgres
  privileges:
  - PUBLIC:
    - all
  - postgres:
    - all

But when I create the corresponding aggregate function, then the error happens:

CREATE AGGREGATE json_agg_test (JSONB)
(
    SFUNC = json_agg_statef,
    STYPE = JSONB
)
;
Traceback (most recent call last):
  File "/usr/local/bin/dbtoyaml", line 11, in <module>
    sys.exit(main())
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbtoyaml.py", line 49, in main
    dbmap = db.to_map()
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/database.py", line 488, in to_map
    dbmap.update(self.db.schemas.to_map(self.db, opts))
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/schema.py", line 357, in to_map
    schemas.update(self[sch].to_map(db, self, opts))
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/schema.py", line 140, in to_map
    schobjs.append((obj, obj.to_map(db, no_owner, no_privs)))
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/function.py", line 569, in to_map
    dct = super(Aggregate, self).to_map(db, no_owner, no_privs)
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/__init__.py", line 361, in to_map
    deps -= self.get_implied_deps(db)
  File "/usr/local/lib/python2.7/dist-packages/pyrseas/dbobject/function.py", line 666, in get_implied_deps
    deps.add(db.functions[sch, fnc, args])
KeyError: (u'public', 'json_agg_statef', u'jsonb, jsonb')

My guess is that the aggregate function is being regarded as a normal function/procedure - which it isn't.

jmafc commented 2 years ago

One last thing: please confirm that you're using the 0.9.1 version of Pyrseas. Josh Curtis (the OP) submitted a PR but it was only committed to master (unfortunately, the PR doesn't fix the bug, but it causes a different behavior).

alexitheodore commented 2 years ago

Confirmed.

postgres@ip-172-33-2-75:~/adt_tmp$ dbtoyaml --version
dbtoyaml 0.9.1
jmafc commented 2 years ago

I've been out of the game for too long it seems. Travis-ci.org doesn't work anymore and travis-ci.com wants me to sign up for a plan and provide a bunch of info, instead of just an email. It also refuses for some strange reason to map perseas/pyrseas as a valid repository. Anyway, I've made a minor change to test_extensions.py so that it can run against PG 13. However, more importantly, the problem that you're seeing is not related to issue #226, which is a problem with the dropped pg_pltempate catalog (which we fetch in yamltodb), but your problem is in dbtoyaml. I'll dig a little further tomorrow and let you know.

jmafc commented 2 years ago

For the record, once I installed plperl-13 and plpython3-13, all five test_map* tests in test_extension.py passed using both Python 2.7 and 3.7.

jmafc commented 2 years ago

If you have access to a database running under PG earlier than 13, then it may be helpful if you ran the test against that. If you don't have access, that's OK. I'll have to download pg_cron and I'll install it on both PG 11 and 13.

jmafc commented 2 years ago

FYI, I tried to install postgresql-11-cron and postgresql-13-cron from the PGDG repositories onto Debian butster, and both failed as "not found". I'm going to try the GH local build instructions.

jmafc commented 2 years ago

@alexitheodore I was able to make pg_cron locally (after installing postgresql-server-{11,13}-dev, which are not mentioned as requirements). The instructions mention /usr/pgsql-12/bin which I'm not sure where it comes from. I tried make --dry-run install and it would try to install lots of stuff all over the place, in particular in /usr/lib/postgresl/13 which I would prefer not to do. IMHO it should it should install under /usr/local/ so it can be removed easily. I'm going to see if there's a way around that. Also I would like to install under PG11 first, because I suspect this is not a PG 13 problem, but rather something unique to pg_cron (dbtoyaml works fine on PG 13 with pg_trgm).

alexitheodore commented 2 years ago

@jmafc

I don't have pg11 handy to test anything with.

I can only add that installation was easy for me on EC2 Ubuntu via:

sudo apt -y install postgresql-13 postgresql-client-13
sudo apt-get -y install postgresql-13-cron

Not sure if that helps - but I completely understand if you don't want to spend all day and muddy your system just to get pg_cron loaded. I also agree, its probably more unique to pg_cron than pg13.

Does pyrseas currently support C language functions?

jmafc commented 2 years ago

I had forgotten to do an apt update. Once I did that, I was able to install pg_cron (postgresql-11-cron) and try it out. I get the same dbtoyaml error (using Python 3, instead of 2.7). It is trying to map the trigger cron_job_cache_invalidate on the table cron.job, but it cannot find the procedure cron.job_cache_invalidate (which does exist). This is related to #209 and the general issue that we retrieve objects that are "owned" by extensions and that therefore should be excluded the map output, but we're not always able to detect that. I think we should open a separate issue for this.

If you'd like to help, you can take a look at the build_dependency_graph queries in pyrseas/database.py (lines 232-298). @dvarrazzo can tell you all about them (sorry Daniele :-). I also think that in the past our "standard" solution has been to add a clause such as seen in dbobject/function.py lines 182-4, i.e., don't load functions that depend on extensions (deptype = 'e'). That's why we're not loading cron.job_cache_invalidate. Perhaps the solution is to add a similar clause to the dbobject/trigger.py (starting at line 94).

The simple answer to your last question is yes, but it may depend on what you think "support" means.

jmafc commented 2 years ago

If you change the WHERE clause of the query in dbobject/trigger.py to the following (basically adding the AND ... NOT IN (...):

            WHERE NOT tgisinternal
              AND (nspname != 'pg_catalog' AND nspname != 'information_schema')
              AND t.tgfoid NOT IN (
                  SELECT objid FROM pg_depend WHERE deptype = 'e'
                               AND classid = 'pg_proc'::regclass)

then dbtoyaml completes without error, at least on my system with PG 11. However, it does list the schema cron with its tables job and job_run_details and those probably shouldn't be there either. I would suggest that you try the above change on your system, trying both your first and second test cases.

jmafc commented 2 years ago

@alexitheodore Did you get a chance to test my suggestion above?

alexitheodore commented 2 years ago

@jmafc Sorry not yet - almost got around to it last night. I will soon, just got swamped.

jmafc commented 2 years ago

Opened new issue (#236) to deal with dbtoyaml problem reported 2-Nov-21.

alexitheodore commented 2 years ago

As posted in #236, the updated SQL fixes the pg_cron bug, though the second error persists:

https://github.com/perseas/Pyrseas/issues/226#issuecomment-958239631

jmafc commented 2 years ago

The discussion from 2 to 13 Nov 2021 is not relevant to the basic issue of yamltodb failing against PG 13. However, as discussed on 7-8 Apr, the PR submitted by @joshcurtis does not resolve the issue. It is not enough to query the pg_available_extensions and pg_language because that only gives the languages that are installed on the target. If, for example (as tested in tests/dbobject/test_extension.py test_create_lang_extension), we have to create/install a new language, e.g., plperl, to later create a new function in that language, the input YAML file has to tells not only that extension plperl is needed, but also that language plperl has to be present, so that when we're connecting the new function we'll find the language. The dropped pg_pltemplate catalog was a surrogate for having dbtoyaml outputting language xxx information to the YAML file(s). Since that is no longer available in PG 13, we can either reconstruct the pg_pltemplate information, i.e., replace the ExtensionDict.from_map() langtempls argument by a hard-coded list of strings (output of SELECT tmplname FROM pg_pltemplate against PG 12 or earlier), or modify dbtoyaml to output the language info (and verify that yamltodb processes it correctly). For now, I'll probably submit a hard-coded solution as a temporay measure.

teejays commented 2 years ago

Hey! Just wanted to share that I am still getting the original error mentioned in this post.

...
psycopg2.errors.UndefinedTable: relation "pg_pltemplate" does not exist
LINE 1: SELECT tmplname FROM pg_pltemplate

I'm on PostgreSQL 14.2, and yamltodb 0.9.1. I am going to try and downgrade my PostgesSQL to 12 (to get over this issue), but wanted to share in case this is helpful. Let me know if anyone needs more info from me on this error.

Turned out pip install Pyrseas was installing a version of Pyrseas without the latest commits. Specifically, the commit was missing. I basically used the instructions to download (using git) and build Pyrseas from source, and that seems to have fixed that error.

jmafc commented 2 years ago

@teejays pip will not install commit 4c1a12e because it was only committed to the master branch but PyPI only has releases up to 0.9.1 (from 2020-4-14). To have pip install the former, we'd have to do a new maintenance release. Sorry.