WIP for a PL/Java API that models PostgreSQL concepts rather than abstracting them away #4
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
That was one I must have written on far too little coffee. Hardly any of it bore any connection to reality. Ah. In my defense, it had been more coherent once, but did not get updated with c9f6a20.
These were not caught in 1fc0be2.
BigDecimal is nearly a fit, as things in the standard Java library go, but it cannot handle NaN or +/- infinity. At any rate, even if this reference implementation of a contract isn't complete with respect to those values, it could easily be wrapped by another contract that could return a subclass of BigDecimal, or a BigDecimal-or-Something ... or some other type that is easily constructed from a BigDecimal. Because PostgreSQL's numeric.c contains the magic numbers, and they aren't exposed in .h files, a regression test is critical to making sure these bits don't rot. In passing, relax the obsessive schema-qualification in TupleTableSlotTest's SQLActions. These are (a) examples, and (b) when run as CI tests, running in a fresh installation, and not all other examples are obsessively schema-qualified, and this one didn't have the operators qualified anyway, and doing *that* makes an example just plain hard to read. # Conflicts: # pljava/src/main/java/org/postgresql/pljava/pg/adt/NumericAdapter.java
As PL/Java 1.6.x requires Java >= 9, it makes little sense for a lingering javadoc comment to say you need Java 1.6 for something.
The need in Java (before Java 15 text blocks, anyway) to break up strings across line breaks and concatenate them has absolutely zero need to be slavishly copied into SQL. There were already working examples (in PassXML, say) that didn't do that. And they were written by me....
Factor the work of lax() into a static method on AdjustingJAXPParser but push the instance methods down to the concrete subclasses, which can return 'this' with the expected type and no unchecked warning.
Addresses PR tada#454.
In older Java versions, it was necessary to pass along the initial javadoc arguments (the ones a file manager would care about) in a separate step before calling the tool. Starting in 19, that's not only no longer necessary, but fails with "--module-source-path specified more than once".
Old one could fail depending on previous matching activity, possibly because of the use of \G (which I should have explained better in a comment, back when I thought I knew why I was doing it). The documented behavior of ^ $ and \z and reluctant quantifiers make for a simpler and more dependable version. Addresses issue tada#455.
Instead of relying on valign to match the parameters to their descriptions, give the tables the class "striped", which is described in the javadoc-supplied stylesheet.css as one of the "Styles for user-provided tables".
It is easy enough to stop casting _PgObject_pureVirtualCalled to incompatible function types, as it makes modern compilers suspicious, and was only done in three places anyway.
introduced in 45f965c.
By allowing occasional gaps in the otherwise-consecutive IDX_... values, new constants can be added as needed, and kept in coherent groupings, with a smaller blast radius in version control (and fewer merge conflicts for other branches or forks), by avoiding extensive renumbering of otherwise untouched members.
Given support for gaps in the ModelConstants IDX_... values, renumber and slightly regroup the constants, with an eye toward reducing the blast radius of future additions when needed.
In backporting, sometimes the git history shows that something has always had the same type, but the type was plain int rather than an explicit-width type. So, for such things, there is no need for a plethora of SIZEOF_FOO constants, but SIZEOF_INT may be generally useful to detect if a platform has a surprising value for that width.
The name andIf could be misread as suggesting some kind of boolean dependency on what went before, when really each alsoIf only cares about its own predicate.
Puzzling how these were overlooked in a7ce56f.
The two drivers use slightly different URL syntax, and different names for some properties (database.name vs. PGDBNAME, and application.name vs. ApplicationName matter here). Expose a new static s_urlForm and constants URL_FORM_PGJDBC and URL_FORM_PGJDBCNG (and URL_FORM_NONE if no recognized driver has been found) so a client can know what's been selected. To simply count the otherwise-unwanted rows of a ResultSet, pgjdbc-ng allows rs.last();rs.getRow(); but PGJDBC does not, at least under its default for ResultSet scrollability. The method voidResultSetDims gets a change here to not attempt to count the rows when called by peek(); instead, it just returns -1 for rows in that case, which will look odd but play well with both drivers. The PostgreSQL backend sends an interesting message in the case of function/procedure creation with types mentioned that are shells. The message has severity NOTICE but SQLState 42809 (ERRCODE_WRONG_OBJECT_TYPE). For some reason, pgjdbc-ng has not been delivering those, but PGJDBC does, and the heuristic of looking at the class digits to decide if it's ok or not would make the wrong call seeing class 42. Happily, PGJDBC exposes (by casting to PGJDBC-specific types) the severity tag from the backend. So that has to be done here, and done reflectively, to avoid a hard PGJDBC dependency. PGJDBC exposes the SEVERITY (S) tag, but not the SEVERITY_NONLOCALIZED (V) tag that was added in PG 10 (postgres/postgres@26fa446). The upshot is that the rule used here to recognize WARNING will fail if the backend is using a language where it's some other word. A new static method set_WARNING_localized can be used to set the correct tag (PERINGATAN in Indonesian, for example) so the classification will happen correctly for the language selected in the backend. For utility statements with no result, where in pgjdbc-ng there really is no result, in PGJDBC there is a zero row count, the same as for a DML statement that touched no rows. It'll take another commit to make the test state machines work either way.
It would be nice to have a little library of common stateMachine states, heretofore difficult because the next state must be indicated by returning an absolute state number. So, use one of the otherwise-unused parameters of the InvocationHandler functional interface to supply a state with its own state number, so it can make relative moves. To start the library, here is NOTHING_OR_PGJDBC_ZERO_COUNT, because the PGJDBC and pgjdbc-ng drivers differ in handling a utility statement with no result (CREATE EXTENSION, that sort of thing). In pgjdbc-ng, there is no result, while in PGJDBC, there is a zero row count, as would be seen for a DML statement that had not updated anything. So, NOTHING_OR_PGJDBC_ZERO_COUNT simply moves to the numerically next state, after consuming (if the driver is pgjdbc-ng) nothing, or (if the driver is PGJDBC) a zero row count.
This is all about convenience when using JShell interactively, where try-with-resources gets in the way. The VM shutdown hooks to stop and clean up the Node work well, but if any Connection is open at the time, the type of shutdown signalled to the server may not cause it to be closed, leading to a hang. The Java Process API does not expose enough types of OS signals to easily request a faster shutdown. (When the node is using pg_ctl, that's an option, but not when signaling the server directly.) So, have each Node remember the Connections it makes, in a WeakHashMap. They'll be removed from the map once unreachable (and if that happens before they're closed, both drivers include a cleaner task that will eventually close them; there may be a bit of delay, but that's better than the former hang). When stopping a Node, close whatever Connections are still around in the map.
Update the docs chiefly to reflect that either PGJDBC or pgjdbc-ng may be used as the JDBC driver, and how to accommodate the slight differences between the two. Numerous other updates, including to more consistently observe the conventional javadoc style where lead sentences are third-person rather than second. In the course of better documenting when tweaks are applied, caught one place where forWindowsCRuntime would not be applied on Windows. (The known arguments being supplied right there in the code could squeak by without it, but who knows what could be added in a tweak?)
Introduced in 17b7d7c.
No issues observed in compiling manually (without the Maven directive specifying an earlier --release) on Oracle JDK 21 GA. cd pljava-api/src/main/java /var/tmp/jdk-21/bin/javac -d ../../../target/classes/ \ -Xlint:unchecked -Xlint:-removal --module-version 1.6-SNAPSHOT \ $(find . -name '*.java') cd ../../.. /var/tmp/jdk-21/bin/jar cf target/pljava-api-1.6-SNAPSHOT.jar \ -C target/classes . cd ../pljava/src/main/java /var/tmp/jdk-21/bin/javac --module-version 1.6-SNAPSHOT \ -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 \ -Xlint:unchecked -Xlint:-removal $(find . -name '*.java') cd ../../../ /var/tmp/jdk-21/bin/jar cf target/pljava-1.6-SNAPSHOT.jar \ -C target/classes . cd ../pljava-examples/src/main/java /var/tmp/jdk-21/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 zip -r ../pljava-examples-1.6-SNAPSHOT.jar * # zip because jar m doesn't preserve order of manifest entries cd ../../../ mvn clean install --projects pljava-packaging install and test
There are still others yet to be implemented, but these are easy enough.
Getting the descriptor from the typcache is handy when a relation has an associated type, but not all kinds of relation have one. An index or TOAST table doesn't, for example. There was already one case that had to be handled by going to the relcache, and that was to get the descriptor for pg_class itself, which can't be expected to find its associated type before it knows what its columns are. So that code path just needs to be used also for the relation kinds that don't have an associated type.
RegClass should have accessors for AccessMethod and Tablespace (Database has a Tablespace also). RegClass indirectly reaches ForeignDataWrapper and ForeignServer. Opting not to make ForeignTable a CatalogObject in its own right: it barely qualifies. It is identified by the oid of the RegClass, and functions more as an extension of that. Its options and ForeignServer can just be given accessors on RegClass. Accessors returning these things to be added in a later commit (along with whitespace-only tidying of lines added here).
This commit includes whitespace-only tidying of lines added in the previous commit.
ForeignTable is simply represented by two accessors added to RegClass. When foreign-table info is wanted, a little class RegClass holds in a single slot gets instantiated and constructs both values. The slot's invalidation still uses the RegClass switch point, rather than also hooking invalidation for pg_foreign_table. In passing, catch up with two pg_database attributes that changed in PG 15 from name to text.
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 tada#524.
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 tada#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.
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 tada#523", so here's that.
The merged work includes PR tada#533, which does away with the old Ptr2Long union in favor of new PointerGetJLong and JlongGet(... conversions. In merging, also convert the uses of Ptr2Long that were added on this branch.
This continues the work started in the REL1_6_STABLE branch of fixing javadoc errors that prevent a successful javadoc run with maximal coverage. This fixes such errors that have been introduced in the org.postgresql.pljava.internal module in this branch.
The only well-known collations pinned with compile-time symbols remaining now are DEFAULT and C (postgres/postgres@51edc4c).
In PG 18, there is now a CompactAttribute struct found in tuple descriptors (postgres/postgres@5983a4c) that contains a field of like purpose, so the one in pg_attribute is gone (postgres/postgres@02a8d0c). PL/Java deforming wasn't making any use of it yet anyway. Where a TupleDesc used to have an attrs offset that was exactly where a sequence of Form_pg_attribute began, it now has a compact_attrs offset where a sequence of CompactAttribute starts. That is still followed by a sequence of Form_pg_attribute, so now the Java code looking for those has to take the compact_attrs offset and add natts * sizeof (CompactAttribute). The CompactAttribute structs were added upstream as a performance optimization, and could perhaps be made use of here to good effect, but for now just compute the offset and use the Form_pg_attribute in the accustomed way. Most promising, perhaps, would be to have TupleDescriptor make use of the attcacheoff member of the new struct.
The earlier work fixing run-busting errors in javadoc comments permits the javadoc coverage for the o.p.p.internal module to be expanded to cover package-private types and members. In passing, add enough missing class-level javadoc comments to make the resulting package listings somewhat presentable.
Interesting that Mac OS clang was the only compiler to spot it.
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.
No description provided.