Modernize for newer PostgreSQL releases - part #1 #3

praiskup · 2018-10-22T12:27:59Z

No description provided.

praiskup · 2018-11-07T15:06:29Z

Anyone wants to review? Otherwise I'd merge this and continue forward, even though this doesn't fix the code entirely.

The testsuite is broken, I am aware of one server crash - and also note this https://postgr.es/m/4738198.8KVIIDhgEB@nb.usersys.redhat.com, we need even more work.

jeltz

Nice to see this project reviewed. I did a quick review of this MR.

It may also be worth taking a look at https://github.com/pg-plruby-ug/postgresql-plruby which seems to be the most modern version of PL/Ruby.

jeltz · 2018-11-10T21:40:35Z

extconf.rb

+       # In v10+, the second number means "minor" version - which is
+       # expected to be >= 10 one day.  So rather mutiply by 100 to avoid
+       # overflow.
+       version = 100 * $1.to_i + $2.to_i


This does not look right. Isn't the version just use for checking for for the major version in the code below? I think something like the below would be better:

when /^PostgreSQL ([7-9]|[1-9][0-9])\.([0-9]{1,3})(\.[0-9]{1,3})?$/ if $1.to_i < 10 # Before v10 the second number was also part of the major version version = 10 * $1.to_i + $2.to_i else version = 10 * $1.to_i

I probably prefer not to expand 10.1 => 100, that would look kind of weird. We usually check like VERSION >= NN anways. But I can admit it is matter of taste. Any other opinion? (I'd probably switch to what @jeltz suggests).

Isn't the version just use for checking for for the major version in the code below?

Plus it is a #define, used in code elsewhere.

I think we should be consistent. In the current master 9.6.1 becomes 96, and in that case 10.1 should become 100. I would be perfectly fine with including the minor version, but in that case we need to do something like 9.6.1 => 90601 and 10.1 => 100001 since we need to support 9.6.11 => 90611 and 10.1 must be larger than 9.6.11.

Also why do we need that #define? PostgreSQL already defines PG_VERSION_NUM. I think I could submit a patch which starts to use PG_VERSION_NUM after you have merged this.

Patch for PG_VERSION_NUM or PG_MAJORVERSION would be awesome.

Turns out such a patch would not be necessary since none of the checks check for anything newer than 8.4.

WIP commit which removes the legacy ifdefs: jeltz@5c65ca1

jeltz · 2018-11-10T21:45:36Z

extconf.rb

@@ -120,8 +120,15 @@ def rule(target, clean = nil)
 end

 case version_str = `#{pg_config} --version`
-when /^PostgreSQL ([7-9])\.([0-9]{1,3})(\.[0-9]{1,3})?$/
-   version = 10 * $1.to_i + $2.to_i
+when /^PostgreSQL ([7-9]|[1-9][0-9])\.([0-9]{1,3})(\.[0-9]{1,3})?$/


This wont work on at least Debian (before or after your patch). My output is "PostgreSQL 11.1 (Debian 11.1-1.pgdg90+1)\n".

Also the regex seems needlessly complicated and will break once we release 10.10. Why not something like: /\APostgreSQL (\d+)\.(\d+)/

We also may want to support development versions like the current "12devel".

Thanks, fixed for the Debian case. Support for development versions can be added later.

jeltz · 2018-11-10T21:49:05Z

extconf.rb

   raise <<-EOT

 ============================================================
-#{version_str} is unsupported.  Try plruby-0.4.2.
+#{version_str} is not supported anymore.  Try plruby-0.5.7 or older.


I think it is rather ugly that this overflows outside the equals signs. I suggest just dropping the "anymore" because it is obvious from the context.

jeltz · 2018-11-10T21:50:27Z

extconf.rb

@@ -132,11 +132,11 @@ def rule(target, clean = nil)
 else
   version = 0
 end
-if version < 73
+if version < 92


If we drop support for anything before 9.2 we should also remove the check against 7.4 below in extconf.rb.

You should also update the README if we increase the max version.

Agreed, but removing everything < 9.2 will be pretty huge cleanup in all the codebase. That said, it is something I'd like to do - but not here in #part1 (nor #part2) - the major thing is to allow successful build of plruby and make the testsuite pass again (start the Travis CI, etc.).

Fair enough. It makes sense to do the clean up in a separate in a commit.

jeltz · 2018-11-10T21:54:59Z

extconf.rb

-      wr << "\t@-rm tmp/* tests/tmp/* 2> /dev/null\n"
-      wr << "\t@rm Makefile\n" if clean
+      wr << "\t-rm -f tmp/* tests/tmp/* 2> /dev/null\n"
+      wr << "\trm Makefile\n" if clean


Shouldn't there be an added -f here?

Probably, thanks.

For versions smaller than 9.6.X, we keep the short '96' (major version) form, and for >= 10 we use the longer form including minor number (e.g. 1003 for 10.3). This fix allows us to keep the old code working, and support newer PG versions.

Previously we called SPI_finish() in pl_trigger_handler(), right before calling the SPI_modifytuple() - but that's not allowed to be called unconnected since PGv10 (commit 1833f1a1c3b0e12b3ea4). So, call SPI_finish() upper in call stack - and share it with pl_trigger_handler().

This is now mandatory on PGv11+ (upstream commit 726117243022178), but it's available only on PGv9.2+. It doesn't seem to be wise to keep compatibility with anything older than 9.2 nowadays anyways so let's avoid #if-else hacks.

jeltz · 2018-11-14T11:58:14Z

src/plruby.c

@@ -363,6 +363,19 @@ pl_protect(plth)
 	    else {
 		retval = pl_func_handler(plth);
 	    }
+
+            PLRUBY_BEGIN_PROTECT(1);


I am not sure what coding style is used for the project but the indentation here does not match the indentation above.

Well, project has no unified whitespace indentation. The current part you talk about has mixed 8-tab width, with 4-space. Most of the code uses 4-space indent, so I followed it even here.

Well, I'd be against re-indentation of the whole project (that only breaks git-blame/bisect attempts). But specifying indentation rules in CONTRIBUTING file would be good thing.

jeltz · 2018-11-14T12:00:45Z

extconf.rb

@@ -68,8 +68,8 @@ def rule(target, clean = nil)
 \tdone;
 "
   if clean != nil
-      wr << "\t@-rm tmp/* tests/tmp/* 2> /dev/null\n"
-      wr << "\t@rm Makefile\n" if clean
+      wr << "\t-rm -f tmp/* tests/tmp/* 2> /dev/null\n"


What is up with the minus before rm? I know it was there before as part of @-rm but should it really be there?

Minus means "ignore non-zero exit status". But with -f it shouldn't be needed, let's drop that.

jeltz · 2018-11-14T12:01:54Z

I think your changes largely look good, minus my minor comments above. Thanks again for reviving this project.

devrimgunduz · 2018-11-16T06:05:08Z

Is this now ready to merge?

praiskup · 2018-11-16T09:33:41Z

Is this now ready to merge?

Hopefully, thanks a lot for the review @jeltz.

praiskup · 2018-11-16T09:35:10Z

@devrimgunduz I can only "merge with merge commit". Would it be possible to configure the project to have "rebase and merge" option?

The heap_formtuple() call has been deprecated for a long time, and finally removed in PGv9.6 by commit 726117243022178e72966cb. The heap_form_tuple() variant exists in PGv8.1+.

Generate 'extconf.h', so we don't have to pass all the -DMACRO=DEF statements through $CPPFLAGS/$CFLAGS. Support newer ruby where rb_frame_last_func() is obsoleted. Include some missing PG headers, and resolve unused/uninitialized warnings. Move from to HeapTupleGetDatum(), supported since PGv8.0.

Also fix argc/argv checks so that only one argument is allowed.

Use 'c' language, not 'C'. Drop 'lancompiler' use.

praiskup force-pushed the build-against-v11 branch from 590c92e to ef6d467 Compare November 7, 2018 15:03

praiskup changed the title ~~Compat with PGv11 after upstream commit 726117243022178~~ Modernize for newer PostgreSQL releases - part #1 Nov 7, 2018

praiskup mentioned this pull request Nov 7, 2018

Is this a main fork? #2

Closed

jeltz reviewed Nov 10, 2018

View reviewed changes

praiskup added 2 commits November 14, 2018 07:59

extconf.rb: detect any PostgreSQL version

a42d2c6

For versions smaller than 9.6.X, we keep the short '96' (major version) form, and for >= 10 we use the longer form including minor number (e.g. 1003 for 10.3). This fix allows us to keep the old code working, and support newer PG versions.

praiskup force-pushed the build-against-v11 branch from f71b7ee to 541b712 Compare November 14, 2018 07:03

Use TupleDescAttr abstraction macro

9c4a24d

This is now mandatory on PGv11+ (upstream commit 726117243022178), but it's available only on PGv9.2+. It doesn't seem to be wise to keep compatibility with anything older than 9.2 nowadays anyways so let's avoid #if-else hacks.

praiskup force-pushed the build-against-v11 branch from 541b712 to 801f6a8 Compare November 14, 2018 07:08

jeltz reviewed Nov 14, 2018

View reviewed changes

jeltz mentioned this pull request Nov 16, 2018

Fix test suite so it runs on modern PostgreSQL versions #7

Merged

praiskup force-pushed the build-against-v11 branch from 801f6a8 to 4041030 Compare November 16, 2018 09:27

praiskup mentioned this pull request Nov 16, 2018

Fix compiler warning about different types #6

Closed

praiskup added 5 commits November 16, 2018 10:44

Use heap_form_tuple instead of heap_formtuple

3074815

The heap_formtuple() call has been deprecated for a long time, and finally removed in PGv9.6 by commit 726117243022178e72966cb. The heap_form_tuple() variant exists in PGv8.1+.

extconf.rb: ignore rm errors in 'make clean'

456beb2

pl_poly_init: drop unused 'closed' variable

c275c43

Also fix argc/argv checks so that only one argument is allowed.

some testsuite fixes

bf8de4d

Use 'c' language, not 'C'. Drop 'lancompiler' use.

praiskup force-pushed the build-against-v11 branch from 4041030 to bf8de4d Compare November 16, 2018 09:44

praiskup merged commit 67f4e4e into devrimgunduz:master Nov 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize for newer PostgreSQL releases - part #1 #3

Modernize for newer PostgreSQL releases - part #1 #3

praiskup commented Oct 22, 2018

praiskup commented Nov 7, 2018 •

edited

jeltz left a comment

jeltz Nov 10, 2018

praiskup Nov 14, 2018

jeltz Nov 14, 2018 •

edited

jeltz Nov 14, 2018 •

edited

praiskup Nov 14, 2018

jeltz Nov 15, 2018

jeltz Nov 10, 2018

praiskup Nov 14, 2018

jeltz Nov 10, 2018

praiskup Nov 14, 2018

jeltz Nov 10, 2018

jeltz Nov 11, 2018

praiskup Nov 14, 2018

jeltz Nov 14, 2018

jeltz Nov 10, 2018

praiskup Nov 14, 2018

jeltz Nov 14, 2018

praiskup Nov 16, 2018

praiskup Nov 16, 2018

jeltz Nov 14, 2018

praiskup Nov 16, 2018

jeltz commented Nov 14, 2018

devrimgunduz commented Nov 16, 2018

praiskup commented Nov 16, 2018

praiskup commented Nov 16, 2018

Modernize for newer PostgreSQL releases - part #1 #3

Modernize for newer PostgreSQL releases - part #1 #3

Conversation

praiskup commented Oct 22, 2018

praiskup commented Nov 7, 2018 • edited

jeltz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeltz Nov 14, 2018 • edited

Choose a reason for hiding this comment

jeltz Nov 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeltz commented Nov 14, 2018

devrimgunduz commented Nov 16, 2018

praiskup commented Nov 16, 2018

praiskup commented Nov 16, 2018

praiskup commented Nov 7, 2018 •

edited

jeltz Nov 14, 2018 •

edited

jeltz Nov 14, 2018 •

edited