Skip to content
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

Allow Clobs to be based on text and Blobs on bytea #1892

Closed
wants to merge 5 commits into from

Conversation

adunstan
Copy link
Contributor

Provide a connection setting lobVarlena, which is false by default.
If true, Clobs are treated as text fields and Blobs as bytea fields,
similarly to how they are treated in MySQL and elsewhere. This makes
it easier to use the same code when dealing with multiple databases.
Comparatively few users use large objects, and this will come more
naturally to many users.

Provide a connection setting lobVarlena, which is false by default.
If true, Clobs are treated as text fields and Blobs as bytea fields,
similarly to how they are treated in MySQL and elsewhere. This makes
it easier to use the same code when dealing with multiple databases.
Comparatively few users use large objects, and this will come more
naturally to many users.
@vlsi
Copy link
Member

vlsi commented Sep 27, 2020

I'm afraid the database itself does not support clob. The difference between clob and text is that clob should support streaming (in and out), and it should support partial read/write.

AFAIK text does not support requests like get me please bytes from 5000 to 10000.

@vlsi
Copy link
Member

vlsi commented Sep 27, 2020

@adunstan , do you think the database could be improved to support streaming APIs for bytea / text?
If that is the case, we would be happy to support that at the driver level.

@adunstan
Copy link
Contributor Author

@adunstan , do you think the database could be improved to support streaming APIs for bytea / text?
If that is the case, we would be happy to support that at the driver level.

Possibly, but I don't see that that needs to be a blocker here.

@vlsi
Copy link
Member

vlsi commented Sep 27, 2020

It is like "adding hints to PostgreSQL".
If "half-baked" clob implementation is added to pgjdbc, then users would think PostgreSQL supports clobs, and they would stop asking the core team to support it.

@adunstan
Copy link
Contributor Author

It is like "adding hints to PostgreSQL".
If "half-baked" clob implementation is added to pgjdbc, then users would think PostgreSQL supports clobs, and they would stop asking the core team to support it.

Well, TBH, as a Postgres committer I don't believe I've ever heard such a request. I'm not sure what that would look like. Except for LOs, which have their own serious problems (e.g. replication), the protocol deals in whole tuples (and whole datums within each tuple).

@vlsi
Copy link
Member

vlsi commented Sep 27, 2020

Here are the sample threads:
https://www.postgresql.org/message-id/48AAA104.EE98.0025.0@wicourts.gov
https://www.postgresql.org/message-id/CAB=Je-EN_Wc-SURAfUAgzLh-kcmxD_migPaO1Jx+SksTAMEuog@mail.gmail.com

I don't want to sound like I am blaming the database, however, I believe we should refrain from implementing non-standard approaches for the standard APIs.

It is like "ok, let's implement a connection property that would treat 31 February as if it was 28 February". Once that property is released, it will be copy-pasted at StackOverflow, and it would be virtually impossible to heal.

Note: I did submit a proposal in "BLOB / CLOB support in PostgreSQL" to pg-hackers 2 years ago, and I got absolutely no replies.
It makes me a sad panda :-(

@adunstan
Copy link
Contributor Author

Here are the sample threads:
https://www.postgresql.org/message-id/48AAA104.EE98.0025.0@wicourts.gov
https://www.postgresql.org/message-id/CAB=Je-EN_Wc-SURAfUAgzLh-kcmxD_migPaO1Jx+SksTAMEuog@mail.gmail.com

I don't want to sound like I am blaming the database, however, I believe we should refrain from implementing non-standard approaches for the standard APIs.

It is like "ok, let's implement a connection property that would treat 31 February as if it was 28 February". Once that property is released, it will be copy-pasted at StackOverflow, and it would be virtually impossible to heal.

I don't think that's a good analogy. I think of it as more of a limitation, in that the driver will have the whole of the datum in memory. I'd be quite happy to add some significant docco explaining the limitations. Would you be happier if we made all the get/set*Stream methods unimplemented?

Note: I did submit a proposal in "BLOB / CLOB support in PostgreSQL" to pg-hackers 2 years ago, and I got absolutely no replies.
It makes me a sad panda :-(

Nothing based on LOs is going to meet the need I am currently trying to meet. The 1GB limitation on text/bytea is not a problem for me, but use of LOs is a show stopper. I could imagine something like a Postgres connection setting that said (say) "only return the first n bytes of a text/bytea datum, and send the rest on demand". But insert/update operations would probably be harder to manage in chunks from streams. I'm just thinking out loud now.

@vlsi
Copy link
Member

vlsi commented Sep 27, 2020

I don't think that's a good analogy. I think of it as more of a limitation, in that the driver will have the whole of the datum in memory

If we make Clob maps to text decision, then we would have a hard time changing that behavior later. That is why I am not very eager making "clob maps to text" decision.

@vlsi
Copy link
Member

vlsi commented Sep 27, 2020

But insert/update operations would probably be harder to manage in chunks from streams. I'm just thinking out loud now.

AFAIK clob/blob does not have insert API. There are update methods that act like overwrite existing content. There's truncate, however, it does not remove contents from the middle of the stream.

Clobs are more challenging to implement since their APIs are character-based, and, as far as I know, TOAST chunks do not contain information on the character length of the chunks.
Then update/overwrite for text contents might result in insert/remove-like behavior if the variable-length encoding is used.

@adunstan
Copy link
Contributor Author

But insert/update operations would probably be harder to manage in chunks from streams. I'm just thinking out loud now.

AFAIK clob/blob does not have insert API. There are update methods that act like overwrite existing content. There's truncate, however, it does not remove contents from the middle of the stream.

Clobs are more challenging to implement since their APIs are character-based, and, as far as I know, TOAST chunks do not contain information on the character length of the chunks.
Then update/overwrite for text contents might result in insert/remove-like behavior if the variable-length encoding is used.

I don't think we can base anything on TOAST - it's supposed to be invisible to the clients.

@adunstan
Copy link
Contributor Author

I don't think that's a good analogy. I think of it as more of a limitation, in that the driver will have the whole of the datum in memory

If we make Clob maps to text decision, then we would have a hard time changing that behavior later. That is why I am not very eager making "clob maps to text" decision.

This patch doesn't preclude another Clob implementation. It's only enabled by a connection setting, which defaults to false (i.e. no change). If you wanted to future proof it a bit more we could change that setting from a boolean to a string, say lobType or something like that. So I disagree with your contention that this would somehow be locking us in.

@roji
Copy link

roji commented Sep 28, 2020

Am just curious about the value of this feature...

AFAIK text does not support requests like get me please bytes from 5000 to 10000.

Isn't that met by substring (which operates on character indexes rather than bytes - but that seems to be a good thing)?

More generally, if the goal is to do random access on text, the functions do seem to meet that need (e.g. overlay for updating as well). I do get that text is limited (to 1GB, right?) compared to what CLOB could provide - but is that the only justification for this?

@vlsi
Copy link
Member

vlsi commented Sep 28, 2020

Isn't that met by substring (which operates on character indexes rather than bytes - but that seems to be a good thing)?

a) User writes query like select id, name, description from my_table. Currently, the database sends full contents, and it is too late for substring.
b) Do you know if select substring(textcol from 1111000 to 111001) from tab reads all the characters from disk or just a couple of them?
c) overlay for updating. Do you mean update my_table set value = overlay(value ...)? Does that mean the database would be able to avoid full-value rewrite in the case of 1 char overlay?

@roji
Copy link

roji commented Sep 28, 2020

a) User writes query like select id, name, description from my_table. Currently, the database sends full contents, and it is too late for substring.

Right, but I'm assuming the user knows before sending the query what substring they want - as they would need to when using any sort of CLOB API... Am I misunderstanding the scenario here?

Re the other two questions... Yes, the overlay with update is what I meant. Your questions are very valid, but that seems to be internal PostgreSQL implementation details. PG does have the information it needs to only load the request substring from disk, and also to update only a single char efficiently with overlay; I'd expect it to do that.

Put another way, it may be more reasonable to ask PG to work efficiently on these random-access text operations, than to use a totally different and dedicated API when these text operations already exist. FWIW the same holds for the already-existing large binary object support - substring and overlay seem to exist for bytea as well, and there's no fundamental reason why they can't be efficient.

Having said all that, the 1GB limit issue still remains.

@vlsi
Copy link
Member

vlsi commented Sep 28, 2020

Vladimir: a) User writes query like select id, name, description from my_table. Currently, the database sends full contents, and it is too late for substring.

Shay: Right, but I'm assuming the user knows before sending the query what substring they want - as they would need to when using any sort of CLOB API... Am I misunderstanding the scenario here?

For instance, Oracle DB returns "clob locator" to the client, so almost any access requires an extra round-trip to the database.
Think of it like the current "LargeObject API".

The client issues select id, name, description_clob ..., and then they can use CLOB API to fetch substrings from the description_clob, and the substrings can result in database roundtrips (e.g. give me please substring ...).

@roji
Copy link

roji commented Sep 28, 2020

The client issues select id, name, description_clob ..., and then they can use CLOB API to fetch substrings from the description_clob, and the substrings can result in database roundtrips (e.g. give me please substring ...).

I feel like I'm missing something (note that everything I'm saying holds for the current PG large object API, not just for the proposed CLOB functionality). Sot how is the substring function with text any worse than the text-based (or bytea-based) substring function? With substring there's no initial API call to fetch any "BLOB/CLOB locator" or identifier, and each substring is fetched in a single roundtrip in both cases, no?

I may be missing some point/requirement with the JDBC CLOB API (which I'm not familiar with).

@vlsi
Copy link
Member

vlsi commented Sep 28, 2020

Sot how is the substring function with text any worse than the text-based (or bytea-based) substring function?

Could you please rephrase?

@bokken
Copy link
Member

bokken commented Sep 28, 2020

@roji, when a large object column is selected, the initial return is just an oid, not the actual content. The PgBlob object is able to take that oid and provide access to the actual content however the consumer wishes (chunks, streaming, entirely in memory, etc.), through additional interactions with the database..

If the columns themselves were defined as text or bytea, the initial return is the entire contents of the column, which would have to be handled in memory by the driver.

@bokken
Copy link
Member

bokken commented Sep 28, 2020

@roji, as I understand what your are proposing, it would require significant statement re-writing to take place to not select the pseudo large object in the initial select through some user supplied hint (presumably on a per query basis). Then when processing the result set, additional queries (using oid?) would have to be done to access the specific column to implement the Blob/Clob/stream/reader apis.

While it /might/ be reasonable to treat all bytea columns this way, it is absolutely not reasonable to treat all text columns this way.

Perhaps the database (and protocol) could be enhanced to define some max length to include inline in response. Anything longer than that would get an indication of being too long and would require subsequent queries to access that data.

@roji
Copy link

roji commented Sep 28, 2020

Sot how is the substring function with text any worse than the text-based (or bytea-based) substring function?

Sorry, I mis-typed. I meant to ask "how is the substring function with text (or bytea) any worse than the proposed CLOB (or existing large binary object) support?"

If the columns themselves were defined as text or bytea, the initial return is the entire contents of the column, which would have to be handled in memory by the driver.

But why does the driver ever have to fetch the entire text/bytea column? If I understand correctly, the JDBC standard forces you to have a "getClob" method, but can't that effectively be a no-op, returning client-side-only data for the TEXT column?

Disregarding the specific shape of JDBC APIs for a second... Say I want to get the slice between characters 1000 and 2000 of some text field.

  1. If we're using the proposed CLOB API (as I understand it from the above), we'd do one roundtrip to get some locator, and then another roundtrip to get the substring.
  2. If we're just using PG text, we'd immediately request the substring.

@davecramer
Copy link
Member

Sot how is the substring function with text any worse than the text-based (or bytea-based) substring function?

Sorry, I mis-typed. I meant to ask "how is the substring function with text (or bytea) any worse than the proposed CLOB (or existing large binary object) support?"

If the columns themselves were defined as text or bytea, the initial return is the entire contents of the column, which would have to be handled in memory by the driver.

But why does the driver ever have to fetch the entire text/bytea column?

As far as I am aware there is no way to get a slice of a text column.

@bokken
Copy link
Member

bokken commented Sep 28, 2020

If the columns themselves were defined as text or bytea, the initial return is the entire contents of the column, which would have to be handled in memory by the driver.

But why does the driver ever have to fetch the entire text/bytea column? If I understand correctly, the JDBC standard forces you to have a "getClob" method, but can't that effectively be a no-op, returning client-side-only data for the TEXT column?

It all depends on what the sql statement is and how the database handles it.

If the sql statement is "select my_large_text from..." the behavior of the database/protocol is to return the entire contents of the column.
If the sql statement is "select substring(my_large_text, 500, 5000) from..." then only the substring is returned.

None of this behavior is controlled by the driver. It is all based on what query the consumer executes. The query is executed and results are in hand before the consumer begins interacting with the ResultSet, which is when the driver gets the first hint that the consumer wishes to interact with a given column as a lob/stream.

@roji
Copy link

roji commented Sep 28, 2020

As far as I am aware there is no way to get a slice of a text column.

Is that not exactly what the substring function does?

If the sql statement is "select my_large_text from..." the behavior of the database/protocol is to return the entire contents of the column.

I guess there is a disconnect here (probably because of me lacking context). What I'm trying to understand, is the value of the existing PG large binary object feature - and of the (PostgreSQL support for the) large text object feature being proposed here.

Users can today perform slicing and various random access operations on simple bytea/text columns. If they execute SELECT bytea_column FROM foo, they're indicating with SQL that they want the entire value. But they can write SELECT substring(bytea_column, ...) FROM foo to indicate they only want a slice of that; AFAIK this works well for both text and binary. Similarly, they can use UPDATE with the overlay function to replace a specific slice of a bytea/text column with some other fragment.

In light of the above, out-of-the-box, existing support for random access operations on bytea/text, what is the proposed added value of a new, different API for CLOB management, and similarly, what is the value of the existing PG large object feature (beyond allowing objects larger than 1GB)?

To be clear, I'm not proposing that the driver rewrite any SQL that the user is providing - just that the user write the SQL they want with the proper functions to perform random-access/slicing operations on text/bytea column, just like they use functions in any other context.

@davecramer
Copy link
Member

As far as I am aware there is no way to get a slice of a text column.

Is that not exactly what the substring function does?

Ah, ok, I was thinking of an API.

If the sql statement is "select my_large_text from..." the behavior of the database/protocol is to return the entire contents of the column.

I guess there is a disconnect here (probably because of me lacking context). What I'm trying to understand, is the value of the existing PG large binary object feature - and of the (PostgreSQL support for the) large text object feature being proposed here.

Users can today perform slicing and various random access operations on simple bytea/text columns. If they execute SELECT bytea_column FROM foo, they're indicating with SQL that they want the entire value. But they can write SELECT substring(bytea_column, ...) FROM foo to indicate they only want a slice of that; AFAIK this works well for both text and binary. Similarly, they can use UPDATE with the overlay function to replace a specific slice of a bytea/text column with some other fragment.

In light of the above, out-of-the-box, existing support for random access operations on bytea/text, what is the proposed added value of a new, different API for CLOB management, and similarly, what is the value of the existing PG large object feature (beyond allowing objects larger than 1GB)?

To be clear, I'm not proposing that the driver rewrite any SQL that the user is providing - just that the user write the SQL they want with the proper functions to perform random-access/slicing operations on text/bytea column, just like they use functions in any other context.

I think notionally the users just want to use hibernate with CLOB's and not worry about what happens under the covers.

@roji
Copy link

roji commented Sep 28, 2020

I think I understand the disconnect on my part. You're trying to allow users (or rather hibernate) to execute SELECT some_clob FROM foo, where some_clob is only an OID, and to use ResultSet.getClob to slice that CLOB in a separate round-trip. So it's an equivalent (and less efficient) method for doing SELECT substring(some_text...) FROM foo, but it's a standard JDBC API which e.g. hibernate uses.

This now makes sense to me, apologies for derailing the conversation. This API still seems problematic to me from a perf perspective, making two roundtrips where one would suffice - but if that's the standard, that's the standard.

On the .NET side, on DbDataReader (the approximate equivalent of ResultSet here), there are indeed GetBytes and GetChars methods, which allow getting slices of the value out. These currently aren't implemented to use the large binary object support - only bytea which downloads everything. They indeed could, though I've never received a request for that.

Sorry once again for the misunderstanding.

@adunstan
Copy link
Contributor Author

I think I understand the disconnect on my part. You're trying to allow users (or rather hibernate) to execute SELECT some_clob FROM foo, where some_clob is only an OID, and to use ResultSet.getClob to slice that CLOB in a separate round-trip. So it's an equivalent (and less efficient) method for doing SELECT substring(some_text...) FROM foo, but it's a standard JDBC API which e.g. hibernate uses.

That's what you can do now with Large Objects. The current proposal is to provide an option to use text fields instead of Large Objects. Note, this is not compulsory, it's an option. And in this case there is no oid and no double round trip. But the whole data value is transferred, and there is a size limit of 1Gb because that's the PostgreSQL limit on text.

@@ -133,6 +133,7 @@ In addition to the standard connection parameters the driver supports a number o
| escapeSyntaxCallMode | String | select | Specifies how JDBC escape call syntax is transformed into underlying SQL (CALL/SELECT), for invoking procedures or functions (requires server version >= 11), possible values: select, callIfNoReturn, call |
| maxResultBuffer | String | null | Specifies size of result buffer in bytes, which can't be exceeded during reading result set. Can be specified as particular size (i.e. "100", "200M" "2G") or as percent of max heap memory (i.e. "10p", "20pct", "50percent") |
| gssEncMode | String | prefer | Controls the preference for using GSSAPI encryption for the connection, values are disable, allow, prefer, and require |
| lobVarlena | Boolean | false | Treat Clobs as text and Blobs as bytea, instead of treating both as LargeObjects |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of the name of this parameter. clobAsText is certainly more direct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm quite open to suggestion, but I think the name needs to reflect the fact that it applies to Blob/bytea as well as Clob/text.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to Blob/bytea as well as Clob/text.

I doubt it makes sense to tie different data types with a single configuration option. What if the user want to map blob=>lo, clob=>text?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what would you like? two boolean settings? I'm OK with that, although I generally try not to proliferate settings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It has to be aligned with the existing settings
  2. It has to be human-friendly
  3. Backend protocol ties encoding to the datatype, and you do even touch that in this PR: connection.getLobVarlena() ? (connection.getStringVarcharFlag().

@@ -0,0 +1,201 @@
/*
* Copyright (c) 2004, PostgreSQL Global Development Group
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use 2020 as the year please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adunstan you were going to fix these ?


private byte[] data;

public static final int MAX_BYTES = 1073741824; // 1GiB max size of a bytea
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason for this to be public ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used in an error check (that I need to complete) in PgPreparedStatement

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if possible protected, even better private.

public static final int MAX_BYTES = 1073741824; // 1GiB max size of a bytea

public PgBlobBytea() {
this.data = new byte[0];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think setting this to null makes more sense as it does not allocate an object. The byte[0] is never used anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started out using null, but the Checker complained loudly, so I got rid of it almost completely in commit c8c1bbf. I'll be happy to revert that, I agree it's more natural, if you can tell me how to keep the checker happy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, revert that, we will fix checker. At worst you can tell it to ignore it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's nothing wrong with using byte @Nullable [] data
Please refer to https://github.com/pgjdbc/pgjdbc/blob/master/CONTRIBUTING.md#null-safety


public PgBlobBytea(byte[] in) throws SQLException {
if (in.length > MAX_BYTES) {
throw new PSQLException(GT.tr("input data too long for bytea type Blob {0}", in.length),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the project capitalizes the first letter in error messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI there are a number of violations of this: try grep -r 'GT.tr[^"]*"[a-z]' pgjdbc/src/main/java/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, we can fix those later. For now lets not add any. Thanks

// starting with the byte specified by pos, which is length bytes in length.
@Override
public InputStream getBinaryStream​(long pos, long length) throws SQLException {
if (this.data == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would data ever be null ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see previous comment re commit c8c1bbf

@@ -0,0 +1,218 @@
/*
* Copyright (c) 2004, PostgreSQL Global Development Group
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2020 same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix

// Retrieves the CLOB value designated by this Clob object as an ascii stream.
@Override
public InputStream getAsciiStream() {
if (this.data != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does data ever get to be null ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the param of PgClobText(String) is null

if (lobVarlena) {
return new PgClobText();
} else {
checkClosed();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should call checkClosed before anything

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix

}

@Override
public Blob createBlob() throws SQLException {
checkClosed();
throw org.postgresql.Driver.notImplemented(this.getClass(), "createBlob()");
if (lobVarlena) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkClosed first

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix

len = inputStream.read(buffer, 0, toRead);
}
if (remaining > 0) {
// not enough bytes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the consequences of this ?

}

if (b.length > PgBlobBytea.MAX_BYTES) {
// XXX throw exception
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should throw the exception here...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix

}

if (b.length > PgBlobBytea.MAX_BYTES) {
// XXX throw exception
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above, please throw the exception

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix

@@ -0,0 +1,269 @@
/*
* Copyright (c) 2005, PostgreSQL Global Development Group
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

year..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix


package org.postgresql.test.jdbc4;

// import static org.junit.Assert.assertEquals;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove these please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix

Comment on lines +68 to +78
assertTrue(clob instanceof PgClobText);
assertTrue(clob.length() == 0);
clob.setString(1,"foobarbaz");
assertTrue(clob.length() == 9);
assertTrue(clob.toString().equals("foobarbaz"));
String s = clob.getSubString(4,3);
assertTrue(s.equals("bar"));
String s2 = clob.getSubString(7,99);
assertTrue(s2.equals("baz"));
long pos = clob.position("bar",1);
assertTrue(pos == 4);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow https://github.com/pgjdbc/pgjdbc/blob/master/CONTRIBUTING.md#test

Please keep tests focused, so one can understand what the test does.
Please refrain from assertTrue usage as it often produces "expected true got false" errors which are hard to reason about.

this.data = s;
}

// This method frees the Clob object and releases the resources the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For implemented methods the javadoc is inherited. There is really no need to repeat the docs here

@adunstan adunstan closed this Mar 9, 2021
@adunstan
Copy link
Contributor Author

adunstan commented Mar 9, 2021

I have an updated version of this that should address (almost) all the issues. I will submit a new PR shortly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants