This PR upgrades PL/Java to version 1.6.10 and adapts it for Cloudberry Database, along with adding a Docker-based CI testing environment. #9
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Now that 1.5.8 is released, the next release should include an extension SQL file allowing upgrade from 1.5.8.
This picks up the PostgreSQL 14 (#359) changes, as well as the #340 (pg_do_encoding_conversion) and #355 (autovacuum) fixes, and some doc mentions of JEP 411. Revised the ErrorData.c patch slightly to avoid a C compiler warning for an unused variable (easier to spot in 1.6 where it's not in a flood of other warnings). That change can be backpatched to 1.5 for consistency.
PL/Java has never, until now, implemented anything by reaching into unexposed JDK innards. The need to do so now is fairly irksome. Java, any language, is classic infrastructure. Other layers, like this, are built atop it, and others in turn use those layers. The idea that the language developers would arrogate to themselves the act of sending an inappropriately low-level message directly to ultimate users, insisting that the stack layers above cannot intercept it and notify the higher-level users in terms that fit the abstractions meaningful there, leaves an uneasy picture of how a development team can begin to lose sight of who is providing what to whom and why.
Adapt beginEnforcing() to throw exceptions with decent explanations on Java > 17 if UnsupportedOperationException has been caught (suggest adding -Djava.security.manager=allow, for whatever good it might do), or if getSecurityManager() returns the wrong thing (suggest trying -Dorg.postgresql.pljava.policy.enforcement=none to ignore the fact, though again this may do little good or lead simply to a different later failure). Add doc comments to various InstallHelper methods lacking them.
Because PL/Java 1.6 builds with --release 9, no @SuppressWarnings annotations need to be added for JEP 411, as the deprecation markings introduced in the JEP are not visible as of release 9. I will turn on the maven-compiler-plugin's <showDeprecation> anyway, as its default of false just seems wrong to me. I'll create a branch from this point that has the @SuppressWarnings annotations that would otherwise need to be added, so they will be on record, but there is no need to merge them in for PL/Java 1.6. May as well not clutter the code with them unnecessarily.
Having suppressed the boilerplate warning from Java itself (which was going to spam standard error for every backend that starts PL/Java, ending up in the server log if logging_collector is set), set about supplying a more usefully worded warning at more strategically chosen times, that will mention the PL/Java wiki/JEP-411 page for details on the road map ahead. The idea is to produce no more than one warning per backend, and only at times when something PL/Java-administrative in nature is happening, to maximize the chance that the person who occasions the warning is a person it will mean something to. So, deliver the warning: 1. when PL/Java itself is installed or upgraded (really, any time its library is loaded by LOAD, directly or by the extension script, even if it's the version already installed and nothing else happens). 2. when functions are declared or redeclared that use it (as detected by the validator handler). 3. when pg_upgrade is run on a database that has PL/Java installed. 1 and 2 are only delivered if the Java major version is 12 or later. The idea is that if a site tends to stick with long-term-support Java releases, there will be plenty of time on 17 LTS for warnings, and no need to deliver them while running 11 LTS. At another site that follows along at a faster pace with non-LTS releases, start with the warnings as soon as running anything later than 11. 3 is unconditional because under pg_upgrade a JVM doesn't get launched to determine its version. 1 and 2 also defer the warning to the commit of a top-level transaction that contained the triggering event(s). If the transaction doesn't commit, the warning is squelched to avoid adding noise to other messages possibly indicating something went wrong.
They should not be counted as ng test results.
Factor the check for a MappedUDT into a checkTypeMappedUDT function, for symmetry with Function_checkTypeBaseUDT. Should make it simpler to grasp what's going on with the two distinct flavors of UDT. Do not expose global functions for obtaining handles for the two support functions that only BaseUDTs have, which are only obtained within Function.c anyway.
Includes a bit of refactoring so internal_call_handler doesn't separately call Function_getFunction and Function_invoke.
This is done from the C side and, perhaps more controversially, using JNI to poke a non-exposed field in java.lang.Thread. PL/Java had not engaged in such tomfoolery before the JEP 411 warning situation required it, and this is now the second instance. It bears mentioning that, from ongoing discussion of JEP 411, the Java developers seem to be taking the position that any project wishing to offer some kind of permission enforcement post-JEP 411 is going to be more or less expected to do so by poking at non-exposed innards using JNI, JVMTI, or the like. In some post-1.6 PL/Java version, this kind of thing will have to become more common. In this case, it is simply to finally start setting the context loader appropriately (I do think it's arguably a correctness issue, as discussed in #361) while adding as little overhead as possible to the hot path of invoking functions. This code will detect, and fall back to not managing the context loader, if the target JRE lacks the expected field. It can also be ineffective if an application has subclassed Thread with a different behavior for getContextClassLoader. It seems tolerable to leave such cases unhandled. Because in PL/Java 1.6 the division of labor still has parameter and return value conversions done from the C side before and after the target method is invoked, and those conversions could involve Java UDTs whose support methods are not bound to context loaders in their own right, it makes sense to have the C code impose the target method's context loader in advance of the parameter conversions, with restoration only after return value conversion. Any UDT methods then consistently see the target method's context loader, whether they are being invoked 'outside' it for parameter or return conversion, or 'inside' it during PreparedStatement / ResultSet / SQLInput / SQLOutput operations. Likewise, leave class initialization to happen, as it normally would, on first use of a function from the class, when the right context loader will be in place. It had been pulled up to validation time to address a small quirk where OpenJ9's class resolution is so lazy it can overlook missing-dependency problems HotSpot would flag, but it's more fuss than the small quirk is worth. This commit introduces some C code in the typedef-function-signature pattern, rather than the typedef-function-pointer pattern prevalent in PostgreSQL upstream, as discussed in [0]. Being valid C89/"ANSI", and with PostgreSQL's documentation stipulating C89 or ANSI compilers as early as PG 8.4 [1], this is expected to cause no problems in an environment that can build any PG version PL/Java 1.6 supports. [0] https://www.postgresql.org/message-id/615C847A.7070609%40anastigmatix.net [1] https://www.postgresql.org/docs/8.4/source-format.html
Slightly increases overhead for the java_thread_pg_entry == allow case. Bottom lines now (X5650, relative to 7a7f394): java_thread_pg_entry == allow: 1.6 µs per call, every call requires setting 0.7 µs per call, consecutive calls use same loader java_thread_pg_entry != allow: 0.7 µs per call, every call requires setting 0.2 µs per call, consecutive calls use same loader
Done as a system property interpreted in InstallHelper, rather than as a GUC, in (a) recognition that it's obscure and may be little used, and (b) anticipation that other allowed settings may evolve in ways that might be easier to parse/validate in Java.
Arguably, these changes should have been made as soon as 1.6.x targeted Java 9 as baseline.
Add a 'builtin' argument (default true) requiring Java to find its own in-the-box XSLT implementation; when false, an implementation via the (now properly managed!) context classloader may be found; for example, XSLT 3.0 transforms can be used if the schema class path includes Saxon. When not using the builtin implementation, treat as warnings, rather than errors, any failure to recognize specific features/attributes the builin implementation is expected to recognize. It's appropriate for the static transformer factory s_tf to be the builtin one.
The earlier validator patch now backed out was motivated not just by the extreme laziness in OpenJ9 (which can happily give you a method handle before even trying to resolve any of the method's symbolic references), but also by getting an unhelpful message even on HotSpot, which is better at detecting such issues, but the exception that really shows what went wrong can be a couple getCause()s down. At least that second issue has an easy solution: just generate a better message.
Because the Java API treats null as a meaningful value for setContextClassLoader, something other than null must be used as the sentinel "didn't set one" value. But choosing some arbitrary int bits other than NULL to use as a sentinel pointer value could be trouble on some architectures. Assuming no logic errors, whatever sentinel value is used here will never get passed to JNI and used as an object reference, but should such an error exist, the results could be undefined unless the value is an actual JNI global reference to a ClassLoader instance. So, to play it safe, just make a dummy one of those to use as the sentinel. If any sequence of mishaps leads Java to use it as a class loader, it will throw null pointer exceptions, but that beats undefined behavior. Invocation_pushBootContext can just use null; its uses may precede the allocation of the sentinel, and popBootContext doesn't try to restore anything anyway.
No functional change.
Check at runtime whether a MappedUDT is being applied to a PostgreSQL type declared with typlen of -2 (a variable-length, NUL-terminated representation), and report an error if so. Explain in the developer documentation why the implementation cannot support the use. It already didn't work, but an explanatory error message is a better way of not working. Addresses #370.
Making this change manually, as dependabot would probably make it in the wrong branch. 1.10 requires Java 8, so no backpatching to REL1_5_STABLE. This whole dependency on ant can probably be removed easily now, by moving the build.xml logic into pom.xml as a scripted-goal.
... in the course of which, noticing that a change being described didn't have enough detail in the JavaDoc, added that too.
Now that 1.6.3 is released, the next release should include an extension SQL file allowing upgrade from 1.6.3.
Now that 1.6.9 is released, the next release should include an extension SQL file allowing upgrade from 1.6.9.
It has grown to such size that breaking it up is long overdue anyway, but the immediate impetus was to do a trial javadoc run checking everything (--show-module-contents all --show-packages-all --show-types private --show-members private). I have no intention of changing the doc build options to build all that as a matter of course, but it ought to be possible, so a run with those settings is useful to identify and fix any javadoc-reported errors that cause the generation to fail, as opposed to mere warnings. Javadoc was immediately displeased by the multi-class compilation unit here.
... with --show-module-contents all --show-packages all --show-types private --show-members private. There are still lots of warnings reported and I have made no effort to look at the docs generated with those settings. But it ought to be possible to do so, with the first step being to eliminate the errors that cause generation to fail.
When VarlenaWrapper wants to save a TOASTed value that may possibly be wanted later, it can use the least memory if it just retains the TOAST pointer and registers a current snapshot in which it is visible. In PG 9.6, a GetOldestSnapshot function appeared and made that possible. As of PG 18, with postgres/postgres@4d82750, that function has vanished, but there is a new one, get_toast_snapshot, that appears to fit the bill. It's only declared in access/toast_internals.h, though, which isn't otherwise needed. Addresses #524.
Merges PR #528.
Merges PR #529.
The comparator could return a premature result if one Snippet or both being compared has a null implementor name, resulting in an output ordering that could depend on the sequence of comparisons performed. The case is unlikely to be seen in practice, as an implementor name can only be made null by explicit implementor="" in an annotation or -Addr.implementor=- when invoking the Java compiler. Nonetheless, the tiebreaker method can be made more simple and convincing by taking advantage of the Java 8 additions to Comparator. Addresses #527. For consistency's sake, also Comparator-ize TypeTiebreaker.
MSVC on GitHub Actions has been reporting consistently that control can reach this point.
PostgreSQL itself has been requiring a C compiler that has uintptr_t since 9.0 (postgres/postgres@85d02a6). The PL/Java 1.6.x branch only supports back to PG 9.5, so there is no question the type is supported. An existing static assert in DualState.c is hereby changed to assert that sizeof(uintptr_t) <= sizeof(jlong); this is also assured for the present by a comment upstream in postgres.h section 1: "we require: sizeof(Datum) == sizeof(void *) == 4 or 8" (where Datum is uintptr_t). In passing, specifically type as SPIPlanPtr some variables that had been typed void *. That was once their actual type in the SPI docs, but not since PG 8.2. The few sites that ended up as PointerGetDatum(JLongGet(Pointer,...)) are effectively a longwinded way of doing nothing, but I left them that way to clearly reflect that a pointer is involved, and avoid assuming PointerGetDatum will always be as simple as it is.
While void * works, mistakes are more easily caught with the intended pointer type.
Merges PR #531.
Merges PR #532.
Merges PR #533.
A not-uncommon mistake by newcomers to PL/Java is to catch an exception raised during a call back into PostgreSQL (such as through the internal JDBC interface), but then to try to proceed without either rolling back to a previously-established Savepoint or (re-)throwing the same or another exception. That leaves the PostgreSQL transaction in an undefined state, and PL/Java will reject subsequent attempts by Java code to call into PostgreSQL again. Those later rejections raise exceptions that may have no discernible connection to the original exception that was mishandled, and may come from completely unexpected places (a ClassNotFoundException from PL/Java's class loader, for example). Meanwhile, the actual exception that was originally mishandled to cause the problem may never be logged or seen, short of connecting with a debugger to catch it when thrown. The result is an overly-challenging troubleshooting process for such a common newcomer mistake. This commit patches PL/Java to retain information about a PostgreSQL error when it is raised and until it is resolved by rolling back to a prior Savepoint or until exit of the PL/Java function. If a subsequent attempt to call into PostgreSQL is rejected because of the earlier error, the exception thrown at that point can supply, with getCause(), the original exception at the root of the problem. If the remembered PostgreSQL error still has not been resolved by a rollback when the function returns (normally or exceptionally) to PostgreSQL, exception stack traces will be logged. The log level depends on whether the function has returned normally or exceptionally and also on whether any later attempts to call into PostgreSQL did get made and rejected. Details are in a new documentation section "Catching PostgreSQL exceptions in Java", which see. Example code is also added.
Also, the first commit neglected to say "Addresses #523", so here's that.
Merges PR #535.
No issues observed in compiling manually (without the Maven directive specifying an earlier --release) on Oracle JDK 25 GA. cd pljava-api/src/main/java /var/tmp/jdk-25/bin/javac -d ../../../target/classes/ \ -Xlint:unchecked -Xlint:-removal --module-version 1.6-SNAPSHOT \ $(find . -name '*.java') cd ../../.. /var/tmp/jdk-25/bin/jar cf target/pljava-api-1.6-SNAPSHOT.jar \ -C target/classes . cd ../pljava/src/main/java /var/tmp/jdk-25/bin/javac --module-version 1.6-SNAPSHOT \ -d ../../../target/classes/ \ -h ../../../target/javah-include \ --module-path ../../../../pljava-api/target/pljava-api-1.6-SNAPSHOT.jar \ --processor-module-path \ ../../../../pljava-api/target/pljava-api-1.6-SNAPSHOT.jar \ -Xlint:unchecked -Xlint:-removal $(find . -name '*.java') cd ../../../ /var/tmp/jdk-25/bin/jar cf target/pljava-1.6-SNAPSHOT.jar \ -C target/classes . cd ../pljava-examples/src/main/java /var/tmp/jdk-25/bin/javac -d ../../../target/classes/ \ --module-path ../../../../pljava-api/target/pljava-api-1.6-SNAPSHOT.jar \ --processor-module-path \ ../../../../pljava-api/target/pljava-api-1.6-SNAPSHOT.jar \ --class-path \ ~/.m2/repository/net/sf/saxon/Saxon-HE/10.9/Saxon-HE-10.9.jar: \ -Xlint:unchecked -Xlint:-removal \ --add-modules org.postgresql.pljava $(find . -name '*.java') cd ../../../target/classes cp -r ../../src/main/resources/* . zip -r ../pljava-examples-1.6-SNAPSHOT.jar * # zip because jar m doesn't preserve order of manifest entries cd ../../../ # with dir of intended pg_config version on PATH: mvn clean install --projects pljava-pgxs,pljava-so,pljava-packaging
Merges PR #542.
mvn versions:set -DgenerateBackupPoms=false -DnewVersion=1.6.10
- Add GitHub Actions workflow to run PL/Java build/test against Cloudberry - Provide reproducible ubuntu22.04 docker-compose environment - Build/install Cloudberry demo cluster inside container - Build/install PL/Java and run built-in regression (make installcheck) - Update expected output for Cloudberry-specific differences
…ry fork. Common ancestor: 5d1dec2 (Poke migration-management versions for 1.5.0) Local branch since ancestor: 198 commits (main_ci@cd6001081) Upstream V1_6_10 since ancestor: 1320 commits (tag d29f8f9) Summary of changes: - Bump project version to 1.6.10 and align Maven/Java build settings (Java 9 release). - Switch native build to pgxs scripted build; remove legacy pljava-deploy and build.xml artifacts. - Sync core runtime C/Java changes (Backend/Session/SPI/Type/module_path, etc). - Update release notes/docs; add upstream CI/config files (Travis/AppVeyor/editorconfig).
- add 1.6.10 extension files and install layout - add legacy example shims for regression - GPDB-specific guards and replicated sqlj tables - adjust loader/fallback behavior - add Cloudberry docker CI and remove lazypg workflow
|
|
Mulily0513
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Key Changes
Change logs
Contributor's checklist
Here are some reminders before you submit your pull request: