New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ugly fix to support PostgreSQL 9.4 #34

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@dev1ant
Copy link

dev1ant commented Jan 12, 2015

First try was in this PR. Now it passes regression tests on both PostgreSQL 9.3 and PostgreSQL 9.4.

@schmiddy

This comment has been minimized.

Copy link
Member

schmiddy commented Jan 14, 2015

Thanks, will try to poke into this as soon as we get 1.3.0 out the door.

@NikolayS

This comment has been minimized.

Copy link

NikolayS commented Feb 6, 2015

Any news about it? 9.4.1 is OU already

@NikolayS

This comment has been minimized.

Copy link

NikolayS commented Feb 6, 2015

*out

@kmoppel

This comment has been minimized.

Copy link

kmoppel commented Feb 19, 2015

+1 :)

@schmiddy

This comment has been minimized.

Copy link
Member

schmiddy commented Feb 27, 2015

Definitely haven't forgotten about this, just haven't had time to make sure this self-described "ugly fix" is OK to go into master. And thanks @dev1ant for putting this PR together and everyone's interest in supporting 9.4.

I wanted to audit the toast index change in 9.4 a bit more closely to make sure there were no surprises in store. For instance, are there any regression tests which would be useful safeguards to add here? Can we check that we really only have one toast index to rebuild and swap instead of possibly multiple indexes which IIRC are theoretically supported now (or may be in a future PG release)?

@dev1ant

This comment has been minimized.

Copy link

dev1ant commented Mar 1, 2015

Well, this fix is ugly because it indeed creates potential problems in the future.To add support for 9.4 beautifully and cleanly much more work (as you mentioned above) should be done. But I have already written that I'm not able to do it :( I've made a patch that works for me right now in a hope that it will help you to make a good fix.

IMHO the code of pg_repack should be divided into branches for different versions of PostgreSQL. Because in lib/pg_repack.sql.in for example you can't write different queries for different version of PostgreSQL. And lots of #if PG_VERSION_NUM ... would be removed and code would be easier to read. But that is really a big change in codebase and more work in maintenance of that code.

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Mar 10, 2015

Conditional support can be introduced no problem from Postgres 9.0 (e.g. using DO and plpgsql, which is installed by default in PG 9.0 servers)

I'm reviewing the 9.4 support and I think we can more easily support it by dropping support for PG 8.4, which is now dropped out of support. So I would release it as pg_repack 1.4. @schmiddy is that ok for you?

@schmiddy

This comment has been minimized.

Copy link
Member

schmiddy commented Mar 10, 2015

Yeah, I think that's alright given that 8.4 is EOL'ed now. IIRC I pushed for us to drop 8.3 support some time ago.

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Mar 10, 2015

Sweet, I'm working on it right now as the pg_reorg 1.1.11 implementation is worrying.... :\

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Mar 10, 2015

Actually, their implementation is not that bad: AFAICS navigating table.reltoastrelid -> toast.oid -> index.indrelid works for PG < 9.4 too (at least I've checked it on PG 9.3), as well as the now unsupported table.reltoastrelid -> toast.reltoastidxid -> index.oid. With an important difference that can be spotted in the server changeset: there could be multiple indexes but only one valid. So apparently pg_reorg 1.1.11 is not broken for PG < 9.4 but still could be wrong on PG 9.4.

If we can support PG 9.4 without introducing 9.0-only dependencies I'd go for a release of pg_repack 1.3.1 instead.

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Mar 10, 2015

@dvarrazzo dvarrazzo closed this Mar 10, 2015

@dev1ant

This comment has been minimized.

Copy link

dev1ant commented Mar 10, 2015

Cool. Thanks!

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Mar 10, 2015

@dev1ant thank you: mine are mostly adjustments on top of your work ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment