Prepared Statement Name Truncation

* Phil Sorber (phil@omniti.com) wrote:

In addition it actually works!

Only until the ORM tries to create two tables that are identical except
for the last few characters..

So I am sharing this with the list to see what people think. Is this a
configuration bug? An ORM bug? A postgres bug? An unfortunate
interaction?

It's a bug that the ORM doesn't correctly handle this particular
per-system limitation cleanly.

If it's a postgres bug, what is the fix? Make the identifier max size
longer? Set a hard limit and ERROR instead of truncating and NOTICE?
Both? Neither because that would break backward compatibility?

Making the identifier longer wouldn't fix anything- an ORM might still
think it can make identifiers longer than whatever PG is configured to
support. Making it unlimited in length would be a huge amount of work
without much gain, imv. ERROR'ing instead of NOTICE'ing is certainly an
option, but it'd break any existing users of this busted ORM or other
similar setups. That's not to say we couldn't do it, but it'd have to
be clearly stated in the release notes and be done as part of the next
major version of PG.

For my part, being very adverse to PG possibly giving the wrong result
(consider INSERT statements which work against a table that doesn't
exist- because the identifier is truncated and happens to end up
matching a table that does exist), I'd advocate changing this to be an
ERROR in 9.3. I don't see the value in the truncation and feel it's
actually dangerous, particularly if ORMs are doing things like this.

Thanks,

Stephen

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

NOTICE: identifier
"this_is_a_really_long_identifier_for_a_prepared_statement_name_ok"
will be truncated to
"this_is_a_really_long_identifier_for_a_prepared_statement_name_"
PREPARE

...

The ORM could use a shorter identifier, but it supports multiple backends
and this is probably not something in their test suite. In addition it
actually works!

For now. If it really works, then by definition it does not /need/ to
be that long, as the truncated version is not blowing things up.

So I am sharing this with the list to see what people think. Is this a
configuration bug? An ORM bug? A postgres bug? An unfortunate
interaction?

Part ORM fault, part Postgres. We really should be throwing something
stronger than a NOTICE on such a radical change to what the user
asked for. I'd lobby for WARNING instead of ERROR, but either way, one
could argue that applications would be more likely to notice and
fix themselves if it was stronger than a NOTICE.

If it's a postgres bug, what is the fix? Make the identifier max size
longer?

I'd also be in favor of this, in addition to upgrading from a NOTICE. We
no longer have any technical reason to keep it NAMEDATALEN, with
the listen/notify rewrite, correct? If so, I'd like to see the max bumped
to at least 128 to match the default SQL spec length for similar items.

Set a hard limit and ERROR instead of truncating and NOTICE?
Both? Neither because that would break backward compatibility?

My vote is WARNING and bump limit to 128 in 9.3. That's the combo most
likely to make dumb applications work better while not breaking
existing smart ones.

- --
Greg Sabino Mullane greg@turnstep.com
End Point Corporation http://www.endpoint.com/
PGP Key: 0x14964AC8 201211172246
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iEYEAREDAAYFAlCoWpYACgkQvJuQZxSWSsi4NwCfQfq7NEQ3xiLpPZLsu0I9iGT4
pOAAmgPEsm2iYCPiVfzMEM2EX2nihQE9
=wLpM
-----END PGP SIGNATURE-----

On 18/11/12 15:46, Stephen Frost wrote:

* Phil Sorber (phil@omniti.com) wrote:

In addition it actually works!

Only until the ORM tries to create two tables that are identical except
for the last few characters..

So I am sharing this with the list to see what people think. Is this a
configuration bug? An ORM bug? A postgres bug? An unfortunate
interaction?

It's a bug that the ORM doesn't correctly handle this particular
per-system limitation cleanly.

If it's a postgres bug, what is the fix? Make the identifier max size
longer? Set a hard limit and ERROR instead of truncating and NOTICE?
Both? Neither because that would break backward compatibility?

Making the identifier longer wouldn't fix anything- an ORM might still
think it can make identifiers longer than whatever PG is configured to
support. Making it unlimited in length would be a huge amount of work
without much gain, imv. ERROR'ing instead of NOTICE'ing is certainly an
option, but it'd break any existing users of this busted ORM or other
similar setups. That's not to say we couldn't do it, but it'd have to
be clearly stated in the release notes and be done as part of the next
major version of PG.

For my part, being very adverse to PG possibly giving the wrong result
(consider INSERT statements which work against a table that doesn't
exist- because the identifier is truncated and happens to end up
matching a table that does exist), I'd advocate changing this to be an
ERROR in 9.3. I don't see the value in the truncation and feel it's
actually dangerous, particularly if ORMs are doing things like this.

Thanks,

Stephen

+1

I hate silent errors that could potential create major problems!

It is far too MySQLish - prepares to duck and run before realizing this
is not a MySQL list! :-)

Maybe a configuration variable to allow unsafe truncation: for those
people who either have no choice, and/or prefer expediency to safety!

Cheers,
Gavin

On 18/11/12 16:49, Greg Sabino Mullane wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

NOTICE: identifier
"this_is_a_really_long_identifier_for_a_prepared_statement_name_ok"
will be truncated to
"this_is_a_really_long_identifier_for_a_prepared_statement_name_"
PREPARE

...

The ORM could use a shorter identifier, but it supports multiple backends
and this is probably not something in their test suite. In addition it
actually works!

For now. If it really works, then by definition it does not /need/ to
be that long, as the truncated version is not blowing things up.

So I am sharing this with the list to see what people think. Is this a
configuration bug? An ORM bug? A postgres bug? An unfortunate
interaction?

Part ORM fault, part Postgres. We really should be throwing something
stronger than a NOTICE on such a radical change to what the user
asked for. I'd lobby for WARNING instead of ERROR, but either way, one
could argue that applications would be more likely to notice and
fix themselves if it was stronger than a NOTICE.

If it's a postgres bug, what is the fix? Make the identifier max size
longer?

I'd also be in favor of this, in addition to upgrading from a NOTICE. We
no longer have any technical reason to keep it NAMEDATALEN, with
the listen/notify rewrite, correct? If so, I'd like to see the max bumped
to at least 128 to match the default SQL spec length for similar items.

Set a hard limit and ERROR instead of truncating and NOTICE?
Both? Neither because that would break backward compatibility?

My vote is WARNING and bump limit to 128 in 9.3. That's the combo most
likely to make dumb applications work better while not breaking
existing smart ones.

[...]

Would it be appropriate to make it a WARNING in 9.2.2, then increase the
length in 9.3?

Though I still feel I'd like it to be an ERROR, may be a configuration
variable in 9.3 to promote it to an ERROR with WARNING being the default?

Cheers,
Gavin

On Nov 17, 2012 11:06 PM, "Gavin Flower" <GavinFlower@archidevsys.co.nz>
wrote:

On 18/11/12 16:49, Greg Sabino Mullane wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

NOTICE: identifier
"this_is_a_really_long_identifier_for_a_prepared_statement_name_ok"
will be truncated to
"this_is_a_really_long_identifier_for_a_prepared_statement_name_"
PREPARE

...

The ORM could use a shorter identifier, but it supports multiple

backends

and this is probably not something in their test suite. In addition it
actually works!

For now. If it really works, then by definition it does not /need/ to
be that long, as the truncated version is not blowing things up.

So I am sharing this with the list to see what people think. Is this a
configuration bug? An ORM bug? A postgres bug? An unfortunate
interaction?

Part ORM fault, part Postgres. We really should be throwing something
stronger than a NOTICE on such a radical change to what the user
asked for. I'd lobby for WARNING instead of ERROR, but either way, one
could argue that applications would be more likely to notice and
fix themselves if it was stronger than a NOTICE.

If it's a postgres bug, what is the fix? Make the identifier max size
longer?

I'd also be in favor of this, in addition to upgrading from a NOTICE. We
no longer have any technical reason to keep it NAMEDATALEN, with
the listen/notify rewrite, correct? If so, I'd like to see the max bumped
to at least 128 to match the default SQL spec length for similar items.

Set a hard limit and ERROR instead of truncating and NOTICE?
Both? Neither because that would break backward compatibility?

My vote is WARNING and bump limit to 128 in 9.3. That's the combo most
likely to make dumb applications work better while not breaking
existing smart ones.

[...]

Would it be appropriate to make it a WARNING in 9.2.2, then increase the

length in 9.3?

Though I still feel I'd like it to be an ERROR, may be a configuration

variable in 9.3 to promote it to an ERROR with WARNING being the default?

In that case I'd make it ERROR by default and make people override to
WARNING if it breaks things. Otherwise no one will change.

On 18/11/12 17:10, Phil Sorber wrote:

On Nov 17, 2012 11:06 PM, "Gavin Flower"
<GavinFlower@archidevsys.co.nz <mailto:GavinFlower@archidevsys.co.nz>>
wrote:

On 18/11/12 16:49, Greg Sabino Mullane wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

NOTICE: identifier
"this_is_a_really_long_identifier_for_a_prepared_statement_name_ok"
will be truncated to
"this_is_a_really_long_identifier_for_a_prepared_statement_name_"
PREPARE

...

The ORM could use a shorter identifier, but it supports multiple

backends

and this is probably not something in their test suite. In addition it
actually works!

For now. If it really works, then by definition it does not /need/ to
be that long, as the truncated version is not blowing things up.

[...]

Set a hard limit and ERROR instead of truncating and NOTICE?
Both? Neither because that would break backward compatibility?

My vote is WARNING and bump limit to 128 in 9.3. That's the combo most
likely to make dumb applications work better while not breaking
existing smart ones.

[...]

Would it be appropriate to make it a WARNING in 9.2.2, then increase

the length in 9.3?

Though I still feel I'd like it to be an ERROR, may be a

configuration variable in 9.3 to promote it to an ERROR with WARNING
being the default?

In that case I'd make it ERROR by default and make people override to
WARNING if it breaks things. Otherwise no one will change.

[...]

How about a WARNING in 9.2.2, and ERROR in 9.3 with a configuration
option to downgrade it to WARNING - as well as increasing the max length
to 128 to match the standard in 9.3 (I assume the size increase is to
drastic for 9.2.x!)?

Cheers,
Gavin

"Greg Sabino Mullane" <greg@turnstep.com> writes:

If it's a postgres bug, what is the fix? Make the identifier max size
longer?

I'd also be in favor of this, in addition to upgrading from a NOTICE.

Increasing NAMEDATALEN has been discussed, and rejected, before. It
is very very far from being a free change: it would double the storage
space required for "name" columns, which is a sizable fraction of the
space eaten in the pg_class and pg_attribute catalogs. (Or we could
convert "name" to a variable length type, but the fallout from that
would vastly exceed what this feature seems worth.)

I think there probably is some case for treating overlength names as
errors not warnings, but on the other hand there's a case for fearing
this will break applications that work fine today. In particular, it
would break applications that expect to be able to use spec-compliant
128-character names, *whether or not they actually have any collisions*.
That seems pretty undesirable, especially in view of the fact that
duplicate-name checks would catch any actual collisions in most cases.

Another point here is that appealing to the letter of the spec in this
area is a bit dubious anyway given the number of ways in which we vary
from exact spec compliance --- notably, allowing non-ASCII characters in
identifiers, allowing dollar signs, allowing leading underscore (no,
that's not per spec), folding to lower case not upper case.

On the whole I'm not too excited about changing this.

regards, tom lane

Phil Sorber <phil@omniti.com> writes:

An install of ours was having an issue with log files filling up the
disk rather quickly. After looking into it, the log was filling up
with NOTICE's caused by an ORM that was using a very long identifier
as a name for a prepared statement. It was a concatenation of tables
in the query.

Just to be clear here ... does this ORM expect that it can concatenate
*any* two legal identifiers into another one? If so, it's going to be
broken no matter what the specific length limit is.

regards, tom lane

On Nov 18, 2012, at 2:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Greg Sabino Mullane" <greg@turnstep.com> writes:

If it's a postgres bug, what is the fix? Make the identifier max size
longer?

I'd also be in favor of this, in addition to upgrading from a NOTICE.

On the whole I'm not too excited about changing this.

Then I'd agree with the OP and think the notice should go away on usage in DML; though it should be kept for DDL.

Can the system be made smart enough to not allow intra-schema collisions in addition to same schema ones? That would seem to be the area of greatest concern - particularly around the usage of truncate/delete/drop.

Thought: would there be some way to flag a table like this to always require the use of a schema prefix to be accessed (since right now truncated names only have to be schema unique) in certain conditions (drop, delete, truncate)?

David J.

On Sun, Nov 18, 2012 at 2:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Phil Sorber <phil@omniti.com> writes:

An install of ours was having an issue with log files filling up the
disk rather quickly. After looking into it, the log was filling up
with NOTICE's caused by an ORM that was using a very long identifier
as a name for a prepared statement. It was a concatenation of tables
in the query.

Just to be clear here ... does this ORM expect that it can concatenate
*any* two legal identifiers into another one? If so, it's going to be
broken no matter what the specific length limit is.

regards, tom lane

I'm not sure if this particular ORM would expect that, but for the
sake of this discussion I think we should assume that whatever ORM
might be interfacing with postgres is doing "the right thing" as it
relates to the spec and we should work to minimize bad interactions. I
assume that was the point of the truncation to begin with, but
unintended consequences and all...

I understand your concern about the cost/benefit of fixing the length
to match spec.

I think making this a warning now would be a bit more forceful way to
let people know that this is a bad idea and this is a case where maybe
they need to work around postgres' lack of conformance to the spec. It
would most likely be caught sooner as well by DBA's. Then in 9.3 we
can make it an error with a GUC to easily override it back to a
warning. So that if it does break people's applications, they can
catch it in testing before an upgrade (assuming they test) and there
is a simple work around so they don't have to change their application
before upgrade. Also with the benefit of being even more forceful in
saying "Don't do that like that!" With a mention in the release notes
of course.

On Sunday, November 18, 2012 at 01:10, David Johnston wrote:

Can the system be made smart enough to not allow intra-schema
collisions in addition to same schema ones? That would seem to be the
area of greatest concern - particularly around the usage of
truncate/delete/drop.

My summary FWIW:
1. Potential exists for internally generated names to exceed maxlen; and
2. this maxlen is shorter than the SQL standard specification; but
3. it may not be worth the performance hit to be SQL compliant in this; with
4. potential for (undetected) name collision and unintended consequences.

May I suggest an idea from the days when memory was counted in (tiny int) kB:
represent the over maxlen identifiers "as is" up to maxlen-8 bytes
use those last 8 bytes for a 40bit hash in Base32 for disambiguation
and,
if 1:10^^12 residual collision risk is considered too high
a side list of overlong names would allow for a second hash disambiguation process.

Notes:
1. The choice of Base32 encoding may be a matter of personal preference
<http://en.wikipedia.org/wiki/Base32&gt;, and, if so, I suggest using the
Crockford encoding <http://www.crockford.com/wrmg/base32.html&gt;.
(I am impressed his design is excellent, while also averting some
accidental obscenities. None of the others offer this feature :)
2. Something along these lines, with the side table to track the
(hopefully) occasional overlong identifiers, could give standards
compliance in identifier length while still keeping the working
tables compact.
3. (Wild speculation) There may be a "sweet spot" using even shorter
identifiers than is the case now, with full disambiguation, which
might improve overall performance.

Regards
Gavan Schneider

Phil Sorber <phil@omniti.com> writes:

I think making this a warning now would be a bit more forceful way to
let people know that this is a bad idea and this is a case where maybe
they need to work around postgres' lack of conformance to the spec. It
would most likely be caught sooner as well by DBA's. Then in 9.3 we
can make it an error with a GUC to easily override it back to a
warning.

Let me be clear here: I don't think we can or should ever make this
into an error by default. Doing that would break spec-compliant
applications, whether or not they are using names that actually have
any conflicts.

There's some possible value in having a non-default option to throw
error for overlength names, but TBH I fear that it won't buy all that
much, because people won't think to turn it on when testing.

Given the historical volume of complaints (to wit, none up to now),
I can't get very excited about changing the behavior here. I think
we're more likely to annoy users than accomplish anything useful.

regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Let me be clear here: I don't think we can or should ever make this
into an error by default. Doing that would break spec-compliant
applications, whether or not they are using names that actually have
any conflicts.

If we increase the length to the spec requirement (full disclosure- I
havn't checked the spec on this myself), I'd be for making it an error
if someone goes over that limit. Right now we have a risk of spec
compliant applications not working under PG for nearly zero good
justification in today's age (iow- I don't really buy the concern about
the size, and if that is a reasonable concern then we could look at
putting in the effort to make it variable length and seriously reduce
today's wasted space).

Regarding the other concerns about how we don't honor the spec
completely- those are situations where we are more flexible and allow
more than the spec says we should. Those are not things which would
break a spec-compliant application, while this is.

There's some possible value in having a non-default option to throw
error for overlength names, but TBH I fear that it won't buy all that
much, because people won't think to turn it on when testing.

I agree that it wouldn't make sense to have an option to make it an
error which is off by default.

Given the historical volume of complaints (to wit, none up to now),
I can't get very excited about changing the behavior here. I think
we're more likely to annoy users than accomplish anything useful.

The lack of compliants is a good reason to not lose sleep over this, I
don't think it's an excuse which allows us to ignore the spec and
applications which adhere to it, if there's someone willing to do the
work to fix it.

I'm not volunteering (yet), but I wouldn't say "no, we're never going to
fix this" either. Perhaps a TODO item to investigate the value of
making relname variable length? Or to investigate the actual impact of
increasing the length?

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

I'm not volunteering (yet), but I wouldn't say "no, we're never going to
fix this" either. Perhaps a TODO item to investigate the value of
making relname variable length? Or to investigate the actual impact of
increasing the length?

My recollection is it's been done already. Try the archives.

regards, tom lane

On 11/19/2012 09:43 PM, Stephen Frost wrote:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Let me be clear here: I don't think we can or should ever make this
into an error by default. Doing that would break spec-compliant
applications, whether or not they are using names that actually have
any conflicts.

If we increase the length to the spec requirement (full disclosure- I
havn't checked the spec on this myself), I'd be for making it an error
if someone goes over that limit. Right now we have a risk of spec
compliant applications not working under PG for nearly zero good
justification in today's age (iow- I don't really buy the concern about
the size, and if that is a reasonable concern then we could look at
putting in the effort to make it variable length and seriously reduce
today's wasted space).

Wasted space is only part of the issue; it's also potentially
significantly cheaper to compare and copy fixed length names, especially
when you can copy on-stack arrays rather than palloc'ing memory.

I have not checked to see whether this is a concern in Pg's codebase;
I'm just aware it's been a reason for the use of fixed length strings in
software in general.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

Gavin Flower asks:

Would it be appropriate to make it a WARNING in 9.2.2, then
increase the length in 9.3?

No: revisions are reserved for bug fixes. This would be more of
a behavior fix and as such would go into a major version.

Gavan Schneider wrote:

(Wild speculation) There may be a "sweet spot" using even shorter
identifiers than is the case now, with full disambiguation, which
might improve overall performance.

I really don't think the length is really a bottleneck, but others
can correct me if it is.

Tom Lane wrote:

There's some possible value in having a non-default option to throw
error for overlength names, but TBH I fear that it won't buy all that
much, because people won't think to turn it on when testing.

Given the historical volume of complaints (to wit, none up to now),
I can't get very excited about changing the behavior here. I think
we're more likely to annoy users than accomplish anything useful.

Well, as with many other things, a lack of complaints does not indicate
there is no problem. I've certainly seen this problem in the wild before,
but have not bothered to file an official bug report or anything. Perhaps
my bad, but the problem is out there. How would you feel about switching
from NOTICE to WARNING, Tom? That seems to make a lot more sense as we
are changing the user's input, which warrants more than a notice IMO.

Separately, what are the objections to raising the size limit to 128?

- --
Greg Sabino Mullane greg@turnstep.com
End Point Corporation http://www.endpoint.com/
PGP Key: 0x14964AC8 201211211525
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iEYEAREDAAYFAlCtOYMACgkQvJuQZxSWSsjmEQCfb6GOEs7jwst1ao70L+j8IW5q
gNYAn110QAhwjuhUSW3/uexvU+StsfZz
=iw6q
-----END PGP SIGNATURE-----

2012/11/21 Greg Sabino Mullane <greg@turnstep.com>:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

Gavin Flower asks:

Would it be appropriate to make it a WARNING in 9.2.2, then
increase the length in 9.3?

No: revisions are reserved for bug fixes. This would be more of
a behavior fix and as such would go into a major version.

Gavan Schneider wrote:

(Wild speculation) There may be a "sweet spot" using even shorter
identifiers than is the case now, with full disambiguation, which
might improve overall performance.

I really don't think the length is really a bottleneck, but others
can correct me if it is.

Tom Lane wrote:

There's some possible value in having a non-default option to throw
error for overlength names, but TBH I fear that it won't buy all that
much, because people won't think to turn it on when testing.

Given the historical volume of complaints (to wit, none up to now),
I can't get very excited about changing the behavior here. I think
we're more likely to annoy users than accomplish anything useful.

Well, as with many other things, a lack of complaints does not indicate
there is no problem. I've certainly seen this problem in the wild before,
but have not bothered to file an official bug report or anything. Perhaps
my bad, but the problem is out there. How would you feel about switching
from NOTICE to WARNING, Tom? That seems to make a lot more sense as we
are changing the user's input, which warrants more than a notice IMO.

Separately, what are the objections to raising the size limit to 128?

significantly larger catalog

Pavel

On 22-11-2012 04:27, Pavel Stehule wrote:

2012/11/21 Greg Sabino Mullane <greg@turnstep.com>: Separately, what are
the objections to raising the size limit to 128?

significantly larger catalog

Less than 5% of catalog columns? I don't buy your argument.

--
Euler Taveira de Oliveira - Timbira http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

2012/11/23 Euler Taveira <euler@timbira.com>:

On 22-11-2012 04:27, Pavel Stehule wrote:

2012/11/21 Greg Sabino Mullane <greg@turnstep.com>: Separately, what are
the objections to raising the size limit to 128?

significantly larger catalog

Less than 5% of catalog columns? I don't buy your argument.

default 6201kB (64) updated 6967kB (128) on empty database - so it is
12% more per one database

in our warehouses we have more than 100000 relations per database and
about 1000 databases per warehouse - so these number can be
significantly higher

second issue can be increasing memory allocations for system caches
and some other place

Regards

Pavel