Обсуждение: DROP relation IF EXISTS Docs and Tests - Bug Fix
This is a follow-up to Bug # 16492 which also links to a thread sent to -hackers back in 2018.
I'm firmly of the belief that the existing behavior of DROP relation IF EXISTS is flawed - it should not be an error if there is a namespace collision but the relkind of the existing relation doesn't match the relkind set by the DROP command.
Since our documentation fails to elaborate on any additional behavior, and uses the relkind in the description, our users (few as they may be) are rightly calling this a bug. I loosely believe that any behavior change in this area should not be back-patched thus for released versions this is a documentation bug. I have attached a patch to fix that bug.
In putting together the patch I noticed that the existing drop_if_exists regression tests exercise the DROP DOMAIN command. Out of curiosity I included that in my namespace testing and discovered that DROP DOMAIN thinks of itself as being a relation for purposes of IF EXISTS but DROP TABLE does not. I modified both DROP DOMAIN and the Glossary in response to this finding - though I suspect to find disagreement with my choice. I looked at pg_class for some guidance but a quick search for RELKIND_ (DOMAIN) and finding nothing decided I didn't know enough and figured to punt on any further exploration of this inconsistency.
The documentation and tests need to go in and be back-patched. After that happens I'll see whether and/or how to go about trying to get my PoV on the behavioral change committed.
David J.
Вложения
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> I'm firmly of the belief that the existing behavior of DROP relation IF
> EXISTS is flawed - it should not be an error if there is a namespace
> collision but the relkind of the existing relation doesn't match the
> relkind set by the DROP command.
I don't particularly agree, as I said in the other thread. The core
point here is that it's not clear to me why the specific error of
"wrong relkind" deserves a pass, while other errors such as "you're
not the owner" don't. Both of those cases suggest that you're not
targeting the relation you think you are, and both of them would get
in the way of a subsequent CREATE. To me, success of DROP IF EXISTS
should mean "the coast is clear to do a CREATE". With an exception
like this, a success would mean nothing at all.
Another point here is that we have largely the same issue with respect
to different subclasses of routines (functions/procedures/aggregates)
and types (base types/composite types/domains). If we do change
something then I'd want to see it done consistently across all these
cases.
regards, tom lane
On Wed, Jun 17, 2020 at 4:32 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> I'm firmly of the belief that the existing behavior of DROP relation IF
> EXISTS is flawed - it should not be an error if there is a namespace
> collision but the relkind of the existing relation doesn't match the
> relkind set by the DROP command.
The other thread:
I don't particularly agree, as I said in the other thread. The core
point here is that it's not clear to me why the specific error of
"wrong relkind" deserves a pass, while other errors such as "you're
not the owner" don't.
Because if you're not the owner then by definition the expected target exists and a drop is attempted - which can still fail.
Both of those cases suggest that you're not
targeting the relation you think you are, and both of them would get
in the way of a subsequent CREATE.
Agreed, as noted on the other thread we actually are not sufficiently paranoid in this situation. Specifically, we allow dropping a relation based upon a search_path search when the target it not on the first entry in the search_path. I'd be glad to see that hole closed up - but this is still broken even when the name is always schema qualified.
To me, success of DROP IF EXISTS
should mean "the coast is clear to do a CREATE". With an exception
like this, a success would mean nothing at all.
To me and at least some users DROP IF EXISTS means that the specific object I specified no longer exists, period.
If you want access to the behavior you describe go and write DROP ROUTINE. As noted on the other thread I think that is a bad option but hey, it does have the benefit of doing exactly what you describe.
Users can write multiple the drop commands necessary to get their create command to execute successfully. If the create command fails they can react to that and figure out where their misunderstanding was. Is that really so terrible?
Another point here is that we have largely the same issue with respect
to different subclasses of routines (functions/procedures/aggregates)
and types (base types/composite types/domains). If we do change
something then I'd want to see it done consistently across all these
cases.
Ok. I don't necessarily disagree. In fact the patch I submitted, which is the on-topic discussion for this thread, brings up the very point that domain behavior here is presently inconsistent.
At least for DROP TABLE IF EXISTS if we close up the hole with search_path resolution by introducing an actual "found relation in the wrong location" error then the risk will have been removed - which exists outside of the IF EXISTS logic - and instead of not dropping a table and throwing an error we just are not dropping a table.
So, in summary, this thread is to document the current behavior [actual doc bug fix]. There is probably another thread buried in all of this for going through and finding other undocumented behaviors for other object types [potential doc bug fixes]. Then a thread for solidifying search_path handling to actually fill in missing seemingly desirable safety features to avoid drop target mis-identification (so we don't actually drop the wrong object) [feature]. Then a thread to discuss whether or not dropping an object that wasn't of the relkind that user specified should be an error [bug fix held up due to insufficient safety features]. Then a thread to discuss DROP ROUTINE [user choice of convenience over safety].
David J.
Hi
čt 18. 6. 2020 v 0:47 odesílatel David G. Johnston <david.g.johnston@gmail.com> napsal:
This is a follow-up to Bug # 16492 which also links to a thread sent to -hackers back in 2018.I'm firmly of the belief that the existing behavior of DROP relation IF EXISTS is flawed - it should not be an error if there is a namespace collision but the relkind of the existing relation doesn't match the relkind set by the DROP command.Since our documentation fails to elaborate on any additional behavior, and uses the relkind in the description, our users (few as they may be) are rightly calling this a bug. I loosely believe that any behavior change in this area should not be back-patched thus for released versions this is a documentation bug. I have attached a patch to fix that bug.In putting together the patch I noticed that the existing drop_if_exists regression tests exercise the DROP DOMAIN command. Out of curiosity I included that in my namespace testing and discovered that DROP DOMAIN thinks of itself as being a relation for purposes of IF EXISTS but DROP TABLE does not. I modified both DROP DOMAIN and the Glossary in response to this finding - though I suspect to find disagreement with my choice. I looked at pg_class for some guidance but a quick search for RELKIND_ (DOMAIN) and finding nothing decided I didn't know enough and figured to punt on any further exploration of this inconsistency.The documentation and tests need to go in and be back-patched. After that happens I'll see whether and/or how to go about trying to get my PoV on the behavioral change committed.
I am reading this patch. I don't think so text for domains and types are correct (or minimally it is little bit messy)
+ This parameter instructs <productname>PostgreSQL</productname> to search
+ for the first instance of any relation with the provided name.
+ If no relations are found a notice is issued and the command ends.
+ If a relation is found then one of two things will happen:
+ if the relation is an domain it is dropped, otherwise the command fails.
+ for the first instance of any relation with the provided name.
+ If no relations are found a notice is issued and the command ends.
+ If a relation is found then one of two things will happen:
+ if the relation is an domain it is dropped, otherwise the command fails.
"If no relations are found ...".
This case is a little bit more complex - domains are not subset of relations. But relations (in Postgres) extends types.
So in this case maybe modified text can be better
+ This parameter instructs <productname>PostgreSQL</productname> to search
+ for the first instance of any domain with the provided name in pg_type catalog.
+ If no type is found a notice is issued and the command ends.
+ If a type is found then one of two things will happen:
+ if the type is a domain it is dropped, otherwise the command fails. Postgres knows
+ for the first instance of any domain with the provided name in pg_type catalog.
+ If no type is found a notice is issued and the command ends.
+ If a type is found then one of two things will happen:
+ if the type is a domain it is dropped, otherwise the command fails. Postgres knows
+ base types, composite types, relation related types and domain types.
Regards
Pavel
David J.
po 13. 7. 2020 v 11:11 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:
Hičt 18. 6. 2020 v 0:47 odesílatel David G. Johnston <david.g.johnston@gmail.com> napsal:This is a follow-up to Bug # 16492 which also links to a thread sent to -hackers back in 2018.I'm firmly of the belief that the existing behavior of DROP relation IF EXISTS is flawed - it should not be an error if there is a namespace collision but the relkind of the existing relation doesn't match the relkind set by the DROP command.Since our documentation fails to elaborate on any additional behavior, and uses the relkind in the description, our users (few as they may be) are rightly calling this a bug. I loosely believe that any behavior change in this area should not be back-patched thus for released versions this is a documentation bug. I have attached a patch to fix that bug.In putting together the patch I noticed that the existing drop_if_exists regression tests exercise the DROP DOMAIN command. Out of curiosity I included that in my namespace testing and discovered that DROP DOMAIN thinks of itself as being a relation for purposes of IF EXISTS but DROP TABLE does not. I modified both DROP DOMAIN and the Glossary in response to this finding - though I suspect to find disagreement with my choice. I looked at pg_class for some guidance but a quick search for RELKIND_ (DOMAIN) and finding nothing decided I didn't know enough and figured to punt on any further exploration of this inconsistency.The documentation and tests need to go in and be back-patched. After that happens I'll see whether and/or how to go about trying to get my PoV on the behavioral change committed.I am reading this patch. I don't think so text for domains and types are correct (or minimally it is little bit messy)+ This parameter instructs <productname>PostgreSQL</productname> to search
+ for the first instance of any relation with the provided name.
+ If no relations are found a notice is issued and the command ends.
+ If a relation is found then one of two things will happen:
+ if the relation is an domain it is dropped, otherwise the command fails."If no relations are found ...".This case is a little bit more complex - domains are not subset of relations. But relations (in Postgres) extends types.So in this case maybe modified text can be better+ This parameter instructs <productname>PostgreSQL</productname> to search
+ for the first instance of any domain with the provided name in pg_type catalog.
+ If no type is found a notice is issued and the command ends.
+ If a type is found then one of two things will happen:
+ if the type is a domain it is dropped, otherwise the command fails. Postgres knows+ base types, composite types, relation related types and domain types.
create type footyp as (a int, b int);
postgres=# drop domain if exists footyp;
ERROR: "footyp" is not a domain
postgres=#
postgres=# drop domain if exists footyp;
ERROR: "footyp" is not a domain
postgres=#
RegardsPavelDavid J.
On Mon, Jul 13, 2020 at 2:12 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
I am reading this patch. I don't think so text for domains and types are correct (or minimally it is little bit messy)This case is a little bit more complex - domains are not subset of relations. But relations (in Postgres) extends types.
Yeah, though in further working on this I dislike the saying "A composite type is a relation" (see Glossary and probably other spots). That a table auto-creates a separate composite type, and depends on it, manifests a certain link between the two but the type that represents the table is not a relation as it doesn't hold data, it is just a definition. If a composite type were a relation then whatever argument you use to justify that would seem to apply to non-composite types as well.
I'm attaching version 2 as a plain diff (complete) instead of a patch.
New with this version is the addition of tests for drop domain and drop type, and related documentation changes. Notably pointing out the fact that DROP TYPE drops all types, including domains.
To recap, the interesting relation related behaviors these tests demonstrate are:
A non-failure while performing a DROP "relation" IF EXISTS command means that a subsequent CREATE "relation" command will not fail due to the name already existing (other failures are of course possible).
In the presence of multiple schemas a failure of a DROP "relation" IF EXISTS command does not necessarily mean that an corresponding CREATE "relation" command would fail - the found entry could belong to a non-first schema on the search_path while the creation will place the newly created object always on the first schema.
The plain meaning of the opposite of "DROP IF EXISTS" (i.e., it's not an error if the specified object doesn't exist, just move on) is not what actually happens but rather we provide an additional test related to namespace occupation that is now documented.
The latter two items are explicitly documented while the first is implicit and self-evident.
David J.
Вложения
út 14. 7. 2020 v 0:37 odesílatel David G. Johnston <david.g.johnston@gmail.com> napsal:
On Mon, Jul 13, 2020 at 2:12 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:I am reading this patch. I don't think so text for domains and types are correct (or minimally it is little bit messy)This case is a little bit more complex - domains are not subset of relations. But relations (in Postgres) extends types.Yeah, though in further working on this I dislike the saying "A composite type is a relation" (see Glossary and probably other spots). That a table auto-creates a separate composite type, and depends on it, manifests a certain link between the two but the type that represents the table is not a relation as it doesn't hold data, it is just a definition. If a composite type were a relation then whatever argument you use to justify that would seem to apply to non-composite types as well.I'm attaching version 2 as a plain diff (complete) instead of a patch.New with this version is the addition of tests for drop domain and drop type, and related documentation changes. Notably pointing out the fact that DROP TYPE drops all types, including domains.To recap, the interesting relation related behaviors these tests demonstrate are:A non-failure while performing a DROP "relation" IF EXISTS command means that a subsequent CREATE "relation" command will not fail due to the name already existing (other failures are of course possible).In the presence of multiple schemas a failure of a DROP "relation" IF EXISTS command does not necessarily mean that an corresponding CREATE "relation" command would fail - the found entry could belong to a non-first schema on the search_path while the creation will place the newly created object always on the first schema.The plain meaning of the opposite of "DROP IF EXISTS" (i.e., it's not an error if the specified object doesn't exist, just move on) is not what actually happens but rather we provide an additional test related to namespace occupation that is now documented.The latter two items are explicitly documented while the first is implicit and self-evident.
I think so now all changes are correct and valuable. I''l mark this patch as ready for commit
Thank you for patch
Regards
Pavel
David J.
On Tue, Jul 14, 2020 at 5:40 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
On Tue, Jul 14, 2020 at 07:25:56AM +0200, Pavel Stehule wrote:
> út 14. 7. 2020 v 0:37 odesílatel David G. Johnston <david.g.johnston@gmail.com> napsal:
> > On Mon, Jul 13, 2020 at 2:12 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> I think so now all changes are correct and valuable. I''l mark this patch
> as ready for commit
This is failing relevant tests in cfbot:
drop_if_exists ... FAILED 450 ms
Oops, did a minor whitespace cleanup in the test file and didn't re-copy expected output. I'm actually going to try and clean up the commenting in the test file a bit to make it easier to read, and split out the glossary changes into their own diff so that the bulk of the changes can be back-patched.
Further comments welcome so I'm putting it back into needs review for the moment while I work on the refactor.
David J.
út 14. 7. 2020 v 15:55 odesílatel David G. Johnston <david.g.johnston@gmail.com> napsal:
On Tue, Jul 14, 2020 at 5:40 AM Justin Pryzby <pryzby@telsasoft.com> wrote:On Tue, Jul 14, 2020 at 07:25:56AM +0200, Pavel Stehule wrote:
> út 14. 7. 2020 v 0:37 odesílatel David G. Johnston <david.g.johnston@gmail.com> napsal:
> > On Mon, Jul 13, 2020 at 2:12 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> I think so now all changes are correct and valuable. I''l mark this patch
> as ready for commit
This is failing relevant tests in cfbot:
drop_if_exists ... FAILED 450 msOops, did a minor whitespace cleanup in the test file and didn't re-copy expected output. I'm actually going to try and clean up the commenting in the test file a bit to make it easier to read, and split out the glossary changes into their own diff so that the bulk of the changes can be back-patched.Further comments welcome so I'm putting it back into needs review for the moment while I work on the refactor.
attached fixed patch
all tests passed
doc build without problems
David J.
Вложения
On Tue, Jul 14, 2020 at 6:56 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
út 14. 7. 2020 v 15:55 odesílatel David G. Johnston <david.g.johnston@gmail.com> napsal:Further comments welcome so I'm putting it back into needs review for the moment while I work on the refactor.attached fixed patchall tests passeddoc build without problems
Thanks.
Actually, one question I didn't pose before, does the SQL standard define DROP TYPE to target domains while also providing for a DROP DOMAIN command? Do drop commands for the other types we have not exist because those aren't SQL standard types (or the standard they are standard types but the commands aren't defined)?
David J.
út 14. 7. 2020 v 16:09 odesílatel David G. Johnston <david.g.johnston@gmail.com> napsal:
On Tue, Jul 14, 2020 at 6:56 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:út 14. 7. 2020 v 15:55 odesílatel David G. Johnston <david.g.johnston@gmail.com> napsal:Further comments welcome so I'm putting it back into needs review for the moment while I work on the refactor.attached fixed patchall tests passeddoc build without problemsThanks.Actually, one question I didn't pose before, does the SQL standard define DROP TYPE to target domains while also providing for a DROP DOMAIN command? Do drop commands for the other types we have not exist because those aren't SQL standard types (or the standard they are standard types but the commands aren't defined)?
It looks like Postgres user defined types are something else than ANSI SQL - so CREATE TYPE and DROP TYPE did different work.
In the section DROP TYPE in ANSI SQL there is not mentioned any relation to domains.
David J.
On Tue, Jul 14, 2020 at 7:21 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
út 14. 7. 2020 v 16:09 odesílatel David G. Johnston <david.g.johnston@gmail.com> napsal:On Tue, Jul 14, 2020 at 6:56 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:út 14. 7. 2020 v 15:55 odesílatel David G. Johnston <david.g.johnston@gmail.com> napsal:Further comments welcome so I'm putting it back into needs review for the moment while I work on the refactor.attached fixed patchall tests passeddoc build without problemsThanks.Actually, one question I didn't pose before, does the SQL standard define DROP TYPE to target domains while also providing for a DROP DOMAIN command? Do drop commands for the other types we have not exist because those aren't SQL standard types (or the standard they are standard types but the commands aren't defined)?It looks like Postgres user defined types are something else than ANSI SQL - so CREATE TYPE and DROP TYPE did different work.In the section DROP TYPE in ANSI SQL there is not mentioned any relation to domains.
Attaching a backpatch-able patch for the main docs and tests, v4
Added a head-only patch for the glossary changes, set to v4 as well.
I didn't try and address any SQL standard dynamics here.
David J.
Вложения
Hi! I've skimmed through the thread and checked the patchset. Everything looks good, except one paragraph, which doesn't look completely clear. + <para> + This emulates the functionality provided by + <xref linkend="sql-createtype"/> but sets the created object's + <glossterm linkend="glossary-type-definition">type definition</glossterm> + to domain. + </para> As I get it states that CREATE DOMAIN somehow "emulates" CREATE TYPE. Could you please, rephrase it? It looks confusing to me yet. ------ Regards, Alexander Korotkov
On Tue, Sep 15, 2020 at 3:48 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
Hi!
I've skimmed through the thread and checked the patchset. Everything
looks good, except one paragraph, which doesn't look completely clear.
+ <para>
+ This emulates the functionality provided by
+ <xref linkend="sql-createtype"/> but sets the created object's
+ <glossterm linkend="glossary-type-definition">type definition</glossterm>
+ to domain.
+ </para>
As I get it states that CREATE DOMAIN somehow "emulates" CREATE TYPE.
Could you please, rephrase it? It looks confusing to me yet.
I'll look at it.
My main point here is that writing "CREATE TYPE typename AS DOMAIN" would be expected, with the appropriate sub-specification, similar to "CREATE TYPE typename AS RANGE". While the syntax wasn't rolled up into "CREATE TYPE" proper "CREATE DOMAIN" effectively does the same thing - creates a type of domain (just ask CREATE TYPE AS RANGE creates a type of range). I'm calling "a type of something" the type's "type domain". CREATE DOMAIN emulates the non-existent "CREATE TYPE typename AS DOMAIN" command.
David J.
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> My main point here is that writing "CREATE TYPE typename AS DOMAIN" would
> be expected, with the appropriate sub-specification, similar to "CREATE
> TYPE typename AS RANGE".
Well, that point seems entirely invented. CREATE DOMAIN is in the
SQL standard:
<domain definition> ::=
CREATE DOMAIN <domain name> [ AS ] <predefined type>
[ <default clause> ]
[ <domain constraint>... ]
[ <collate clause> ]
While SQL does also have a CREATE TYPE command, domains are not
among the kinds of type it can make. So that separation is
very much per spec.
I don't personally find the doc changes proposed here to be a good idea.
001 seems to add a lot of verbosity and not much else. 002 invents terms
used nowhere else in our docs, which seems more confusing than anything
else. It is very badly in need of copy-editing, as well.
Also, I think the phrase you are looking for might be "type category".
Using "type definition" to mean that seems completely wrong. Deciding
that capitalized Type means something special is something I might expect
to find in one of the more abstruse philosophers, but it's not a great
idea in the Postgres manual ... especially when you then use different
terminology elsewhere.
regards, tom lane
On Wed, Sep 16, 2020 at 4:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> My main point here is that writing "CREATE TYPE typename AS DOMAIN" would
> be expected, with the appropriate sub-specification, similar to "CREATE
> TYPE typename AS RANGE".
Well, that point seems entirely invented. CREATE DOMAIN is in the
SQL standard:
<domain definition> ::=
CREATE DOMAIN <domain name> [ AS ] <predefined type>
[ <default clause> ]
[ <domain constraint>... ]
[ <collate clause> ]
While SQL does also have a CREATE TYPE command, domains are not
among the kinds of type it can make. So that separation is
very much per spec.
I don't personally find the doc changes proposed here to be a good idea.
001 seems to add a lot of verbosity and not much else.
The intent is to add accuracy, which means verbosity given the non-obvious choice made in the current implementation.
002 invents terms
used nowhere else in our docs, which seems more confusing than anything
else.
Fair point - was hoping it would be discussion starter.
It is very badly in need of copy-editing, as well.
I'll look at it with fresh eyes...
Also, I think the phrase you are looking for might be "type category".
Actually what I want is "Type type (typtype)" according to pg_type but that seemed like an implementation detail that would be undesirable to use here so I tried to give it a different name. Type category (typcategory) already has a meaning.
Using "type definition" to mean that seems completely wrong. Deciding
that capitalized Type means something special is something I might expect
to find in one of the more abstruse philosophers, but it's not a great
idea in the Postgres manual ... especially when you then use different
terminology elsewhere.
I very well may have been inconsistent but coupled with the above point "type of the Type" seems easier to follow compared to "type of the type" if I were to change "type definition" to "type of the Type".
David J.
On Wed, Sep 16, 2020 at 4:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> My main point here is that writing "CREATE TYPE typename AS DOMAIN" would
> be expected, with the appropriate sub-specification, similar to "CREATE
> TYPE typename AS RANGE".
Well, that point seems entirely invented. CREATE DOMAIN is in the
SQL standard:
And I'm writing for the user who sees that both "CREATE DOMAIN" and "CREATE TYPE AS RANGE" exist, and that there is no "CREATE RANGE", and wonders why if domains are simply a variant of a type, like ranges are, why doesn't CREATE TYPE just create those as well - or, rather, are there any material differences. I choose to include an observation that, no, they are not materially different in terms of being abstract types.
It struck me as odd that it wasn't just CREATE TYPE AS DOMAIN and so in my patch I thought to comment upon the oddity - and in doing so emphasize that the DROP behavior for DOMAINS is no different than the types created by the CREATE TYPE command.
David J.
On Tue, Sep 15, 2020 at 3:48 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
Hi!
I've skimmed through the thread and checked the patchset. Everything
looks good, except one paragraph, which doesn't look completely clear.
+ <para>
+ This emulates the functionality provided by
+ <xref linkend="sql-createtype"/> but sets the created object's
+ <glossterm linkend="glossary-type-definition">type definition</glossterm>
+ to domain.
+ </para>
As I get it states that CREATE DOMAIN somehow "emulates" CREATE TYPE.
Could you please, rephrase it? It looks confusing to me yet.
v5 attached, looking at this fresh and with some comments to consider.
I ended up just combining both patches into one.
I did away with the glossary changes altogether, and the invention of the new term. I ended up limiting "type's type" to just domain usage but did a couple of a additional tweaks that tried to treat domains as not being actual types even though, at least in PostgreSQL, they are (at least as far as DROP TYPE is concerned - and since I don't have any understanding of the SQL Standard's decision to separate out create domain and create type I'll just stick to the implementation in front of me.
David J.
Вложения
st 30. 9. 2020 v 4:01 odesílatel David G. Johnston <david.g.johnston@gmail.com> napsal:
On Tue, Sep 15, 2020 at 3:48 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:Hi!
I've skimmed through the thread and checked the patchset. Everything
looks good, except one paragraph, which doesn't look completely clear.
+ <para>
+ This emulates the functionality provided by
+ <xref linkend="sql-createtype"/> but sets the created object's
+ <glossterm linkend="glossary-type-definition">type definition</glossterm>
+ to domain.
+ </para>
As I get it states that CREATE DOMAIN somehow "emulates" CREATE TYPE.
Could you please, rephrase it? It looks confusing to me yet.v5 attached, looking at this fresh and with some comments to consider.I ended up just combining both patches into one.I did away with the glossary changes altogether, and the invention of the new term. I ended up limiting "type's type" to just domain usage but did a couple of a additional tweaks that tried to treat domains as not being actual types even though, at least in PostgreSQL, they are (at least as far as DROP TYPE is concerned - and since I don't have any understanding of the SQL Standard's decision to separate out create domain and create type I'll just stick to the implementation in front of me.
+1
Pavel
David J.
On 30.09.2020 05:00, David G. Johnston wrote:
On Tue, Sep 15, 2020 at 3:48 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:Hi!
I've skimmed through the thread and checked the patchset. Everything
looks good, except one paragraph, which doesn't look completely clear.
+ <para>
+ This emulates the functionality provided by
+ <xref linkend="sql-createtype"/> but sets the created object's
+ <glossterm linkend="glossary-type-definition">type definition</glossterm>
+ to domain.
+ </para>
As I get it states that CREATE DOMAIN somehow "emulates" CREATE TYPE.
Could you please, rephrase it? It looks confusing to me yet.v5 attached, looking at this fresh and with some comments to consider.I ended up just combining both patches into one.I did away with the glossary changes altogether, and the invention of the new term. I ended up limiting "type's type" to just domain usage but did a couple of a additional tweaks that tried to treat domains as not being actual types even though, at least in PostgreSQL, they are (at least as far as DROP TYPE is concerned - and since I don't have any understanding of the SQL Standard's decision to separate out create domain and create type I'll just stick to the implementation in front of me.David J.
Reminder from a CF manager, as this thread was inactive for a while.
Alexander, I see you signed up as a committer for this entry. Are you going to continue this work?
-- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
On Tue, Mar 9, 2021 at 9:01 PM David Steele <david@pgmasters.net> wrote:
On 3/9/21 10:08 AM, David G. Johnston wrote:
>
> On Tuesday, March 9, 2021, David Steele <david@pgmasters.net
> <mailto:david@pgmasters.net>> wrote:
>
> Further, I think we should close this entry at the end of the CF if
> it does not attract committer interest. Tom is not in favor of the
> patch and it appears Alexander decided not to commit it.
>
> Pavel re-reviewed it and was fine with ready-to-commit so that status
> seems fine.
Ah yes, that was my mistake.
Regards,
--
-David
david@pgmasters.net
and @David Steele comments, it's not clear whether it should be "Read for commit" or "Need Review".
--
Ibrar Ahmed
On Tue, Jul 13, 2021 at 3:30 AM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:
On Tue, Mar 9, 2021 at 9:01 PM David Steele <david@pgmasters.net> wrote:On 3/9/21 10:08 AM, David G. Johnston wrote:
>
> On Tuesday, March 9, 2021, David Steele <david@pgmasters.net
> <mailto:david@pgmasters.net>> wrote:
>
> Further, I think we should close this entry at the end of the CF if
> it does not attract committer interest. Tom is not in favor of the
> patch and it appears Alexander decided not to commit it.
>
> Pavel re-reviewed it and was fine with ready-to-commit so that status
> seems fine.
Ah yes, that was my mistake.
Regards,
--
-David
david@pgmasters.netThe status of the patch is "Need Review" which was previously "Ready for Committer ''. After @David G
I changed it to Ready to Commit based on the same logic as my reply to David quoted above.
David J.
On Tue, Mar 9, 2021 at 10:09 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:
> Frankly, I am hoping for a bit more constructive feedback and even collaboration from a committer, specifically Tom,
onthis one given the outstanding user complaints received on the topic, our disagreement regarding fixing it (which
motivatesthe patch to better document and add tests), and professional courtesy given to a fellow consistent community
contributor.
>
> So, no, making it just go away because one of the dozens of committers can’t make time to try and make it work
doesn’tsit well with me. If a committer wants to actively reject the patch with an explanation then so be it.
I have reviewed this patch and my opinion is that we should reject it,
because in my opinion, the documentation changes are not improvements,
and there is no really clear need for the regression test additions.
According to my view of the situation, there are three kinds of
changes in this patch. The first set of hunks make minor wording
adjustments. Typical is this:
<command>CREATE DOMAIN</command> creates a new domain. A domain is
- essentially a data type with optional constraints (restrictions on
+ a data type with optional constraints (restrictions on
the allowed set of values).
So, the only change here is deleting the word "essentially." Now, it's
possible that if someone different had written the original text, they
might have chosen to leave that word out. Personally, I would have
chosen to include it, but it's a judgement call. However, I find it
extremely difficult to imagine that anybody will be confused because
of the presence of that word there.
- The domain name must be unique among the types and domains existing
- in its schema.
+ The domain name must be unique among all types (not just domains)
+ existing in its schema.
Similarly here. It is arguable which way the text reads better, but
the stated purpose of the patch is to make the behavior more clear,
and I cannot imagine someone reading the existing text and getting
confused, and then reading the patched text and being not confused.
- Do not throw an error if the domain does not exist. A notice is issued
- in this case.
+ This parameter instructs <productname>PostgreSQL</productname> to search
+ for the first instance of any type with the provided name.
+ If no type is found a notice is issued and the command ends.
+ If a type is found then one of two things will happen:
+ if the type is a domain it is dropped, otherwise the command fails.
This is the second kind of hunk that is present in this patch. There
are a whole bunch that are very similar, adjusting the documentation
for various object types. I appreciate that it does confuse users
sometimes that a DROP IF NOT EXISTS command can still fail for some
other reason, so maybe we should try to clarify that in some way, but
I find this explanation to be too complex and technical to be helpful.
If we feel it's necessary to be more clear here, I'd suggest keeping
the existing text and adding a sentence like: "Note that the command
can still fail for other reasons; for example, it is an error if a
type with the provided name exists but is not a domain."
I feel that it's unnecessary to try to clarify what the behavior of
the command is relative to search_path, but if it were agreed that we
need to do so, I still would not like this way of doing it. First, you
never actually use the term "search_path". I expect a lot of people
would be confused by the reference to searching "for the first
instance" because they won't know what is being searched. Second, this
entire bit of text is inside the description of "IF NOT EXISTS" but
the behavior in question is mostly stuff that applies whether or not
"IF NOT EXISTS" is specified. Moreover, it's not only not specific to
IF NOT EXISTS, it's not even specific to this particular command. The
behavior of looking through the search_path for the first instance of
some object type is one that we use practically everywhere in all
kinds of situations. I think we are going to go crazy if we try to
re-explain that in every place where it might be relevant.
The final portion of the patch adds new regression tests. I'm not
going to say that this is completely without merit, because it can be
useful to know if some patch changes the behavior, but I guess I don't
really see a whole lot of value in it, either. It's easy to end up
with a ton of tests that take up a little bit of time every time
someone runs 'make check' but only ever find behavior changes that the
developer was intentionally trying to make. I think it's possible that
these changes would end up falling into that category. I wouldn't
complaining about them getting committed, or about committing them
myself, if there were a chorus of people convinced that they were
worth having, but there isn't, and I don't find them valuable enough
myself to justify a commit.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Tue, Aug 10, 2021 at 12:04 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Mar 9, 2021 at 10:09 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:
> Frankly, I am hoping for a bit more constructive feedback and even collaboration from a committer, specifically Tom, on this one given the outstanding user complaints received on the topic, our disagreement regarding fixing it (which motivates the patch to better document and add tests), and professional courtesy given to a fellow consistent community contributor.
>
> So, no, making it just go away because one of the dozens of committers can’t make time to try and make it work doesn’t sit well with me. If a committer wants to actively reject the patch with an explanation then so be it.
I have reviewed this patch and my opinion is that we should reject it,
Thank you for the feedback.
So, the only change here is deleting the word "essentially."
I do tend to find this wishy-washy language to be more annoying than the community at large.
I'd suggest keeping
the existing text and adding a sentence like: "Note that the command
can still fail for other reasons; for example, it is an error if a
type with the provided name exists but is not a domain."
I would at least like to see this added in response to the various bug reports that found the shared namespace among types, and the fact that it causes an error, to be a surprise.
The final portion of the patch adds new regression tests. I'm not
going to say that this is completely without merit, because it can be
useful to know if some patch changes the behavior, but I guess I don't
really see a whole lot of value in it, either.
I'd say the Bug # 16492 tests warrant keeping independent of the opinion that demonstrating the complicated interplay between 10+ SQL commands isn't worth the test suite time. I'd say that probably half of the tests are demonstrating non-intuitive behavior from my perspective. The bug test noted above plus the one the demonstration that a table in the non-first schema in a search_path will not prevent a create <type> command from succeeding but will cause a DROP <type non-table> IF EXISTS to error out. Does it need to test all 5 types, probably not, but it should at least test DROP VIEW IF EXISTS test_rel_exists.
What about the inherent confusion that having both DROP DOMAIN when DROP TYPE will also drop domains? The doc change for that doesn't really fit into your buckets. It would include:
drop_domain.sgml
+ This duplicates the functionality provided by
+ <xref linkend="sql-droptype"/> but restricts
+ the type's type to domain.
+ <xref linkend="sql-droptype"/> but restricts
+ the type's type to domain.
drop_type.sgml
+ This includes domains, though they can be removed specifically
+ by using the <xref linkend="sql-dropdomain"/> command.
+ by using the <xref linkend="sql-dropdomain"/> command.
Adding sql-droptype to "See Also" on all the domain related command pages as well.
After looking at this again I will say I do agree that the procedural nature of the doc changes for the main issue were probably overkill and a "oh-by-the-way" note as to the fact that we ERROR on a namespace conflict would address that concern in a user-facing way adequately. Looking back and what I went through to put the test script together I don't regret doing the work and feel that someone like myself would benefit from its existence as a whole. It's more useful than a README that would communicate the same information.
David J.
On Tue, Aug 10, 2021 at 5:53 PM David G. Johnston <david.g.johnston@gmail.com> wrote: >> The final portion of the patch adds new regression tests. I'm not >> going to say that this is completely without merit, because it can be >> useful to know if some patch changes the behavior, but I guess I don't >> really see a whole lot of value in it, either. > I'd say the Bug # 16492 tests warrant keeping independent of the opinion that demonstrating the complicated interplay between10+ SQL commands isn't worth the test suite time. I'd say that probably half of the tests are demonstrating non-intuitivebehavior from my perspective. The bug test noted above plus the one the demonstration that a table in the non-firstschema in a search_path will not prevent a create <type> command from succeeding but will cause a DROP <type non-table>IF EXISTS to error out. Does it need to test all 5 types, probably not, but it should at least test DROP VIEWIF EXISTS test_rel_exists. It's not the function of the regression tests to demonstrate behavior. Nobody says "I wonder how the system works, I guess I'll go look at the regression tests," or at least very few people. The job of the regression test is to let us know when things get broken by future changes to the source code, or in other words, to find regressions. > What about the inherent confusion that having both DROP DOMAIN when DROP TYPE will also drop domains? The doc change forthat doesn't really fit into your buckets. It would include: > > drop_domain.sgml > + This duplicates the functionality provided by > + <xref linkend="sql-droptype"/> but restricts > + the type's type to domain. > > drop_type.sgml > + This includes domains, though they can be removed specifically > + by using the <xref linkend="sql-dropdomain"/> command. Maybe we could add a sentence to the end of the DROP DOMAIN synopsis, like: Since a domain is a kind of type, they can alternatively be dropped using DROP TYPE. And for DROP TYPE, we could say something like: Since a domain is a kind of type, it is possible to drop a domain using this command rather than DROP DOMAIN. I think that's similar to your proposal, but I prefer it because I think the duplicate-but-restrict language makes it sound kind of dumb that DROP DOMAIN exists at all, and I don't think it's dumb that we have DROP commands matching the CREATE commands that we also have. > Adding sql-droptype to "See Also" on all the domain related command pages as well. I probably wouldn't do this, but the notes above could include cross-links. > After looking at this again I will say I do agree that the procedural nature of the doc changes for the main issue wereprobably overkill and a "oh-by-the-way" note as to the fact that we ERROR on a namespace conflict would address thatconcern in a user-facing way adequately. Looking back and what I went through to put the test script together I don'tregret doing the work and feel that someone like myself would benefit from its existence as a whole. It's more usefulthan a README that would communicate the same information. Yeah. I tend to feel like this is the kind of thing where it's not likely to be 100% obvious to users how it all works no matter what we put in the documentation, and that some of the behavior of a system has to be learned just by trying out the system. Now, that doesn't mean that we should be immune to ideas about how to improve it, just that someone can always have a question that isn't answered there. -- Robert Haas EDB: http://www.enterprisedb.com
On Wed, Aug 11, 2021 at 5:54 AM Robert Haas <robertmhaas@gmail.com> wrote:
Yeah. I tend to feel like this is the kind of thing where it's not
likely to be 100% obvious to users how it all works no matter what we
put in the documentation, and that some of the behavior of a system
has to be learned just by trying out the system.
I see where you are coming from and agree with a good portion of it, and accept that you are accurately representing reality for most of the rest, regardless of my feelings on that reality. I'm not invested in this enough to try and change mindsets. I have withdrawn the patch from the commitfest and do not plan on putting forth a replacement.
Thank you for your thorough review, I do appreciate it. It did reaffirm some of the suspicions I had about the wording I had chosen at least.
I will add that when I finished the patch I felt it was of more value to future developers than it would be to our end users. I never really worried that a patch could be written to fill in the missing pieces that prompted the various bug reports. I went to the extra effort because the underlying interactions seemed complicated and I wanted to see in practice exactly how they behaved and what that meant for usability. I also still disagree with our choice to emit an error on a namespace collision, and generally feel this area could use improvement. Thus I keep the tests around, which basically communicate that "this is how things work and it is intentional" and also are useful to have should future changes, however unlikely to materialize, be made. Sure, that isn't "regression" but in my unfamiliarity I didn't know of a better existing place to put them and didn't think to figure out (or create) a better location, one that doesn't run on every build.
David J.