Обсуждение: more ALTER .. DEPENDS ON EXTENSION fixes

ALTER ... DEPENDS ON EXTENSION (dependencies of type 'x' on an
extension) was found to have a few problems.  One was fixed as
CVE-2020-1720.  Other issues:

* pg_dump does not reproduce database state correctly.
  The attached 0000 fixes it.

* More than one 'x' dependencies are allowed for the same object on the
  same extension.  That's useless and polluting, so should be prevented.

* There's no way to remove an 'x' dependency.

I'll send patches for the other two issues as replies later.  (I
discovered an issue in my 0001, for the second one, just as I was
sending.)

-- 
Álvaro Herrera

On 2020-Feb-17, Alvaro Herrera wrote:

> * More than one 'x' dependencies are allowed for the same object on the
>   same extension.  That's useless and polluting, so should be prevented.
> 
> * There's no way to remove an 'x' dependency.

Here's these two patches.  There's an "if (true)" in 0002 which is a
little weird -- that's there just to avoid reindenting those lines in
0003.

In principle, I would think that these are all backpatchable bugfixes.
Maybe 0002 could pass as not backpatchable since it disallows a command
that works today.  OTOH the feature is rarely used, so maybe a backpatch
is not welcome anyhow.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            not tested

Tested the pg_dump patch for dumping "ALTER .. DEPENDS ON EXTENSION" in case of indexes, functions, triggers etc. The
"ALTER.. DEPENDS ON EXTENSION" is included in the dump. However in some case not sure why "ALTER INDEX.....DEPENDS ON
EXTENSION"is repeated several times in the dump? 

On 2020-Feb-28, ahsan hadi wrote:


> Tested the pg_dump patch for dumping "ALTER .. DEPENDS ON EXTENSION" in case of indexes, functions, triggers etc. The
"ALTER.. DEPENDS ON EXTENSION" is included in the dump. However in some case not sure why "ALTER INDEX.....DEPENDS ON
EXTENSION"is repeated several times in the dump?
 

Hi, thanks for testing.

Are the repeated commands for the same index, same extension?  Did you
apply the same command multiple times before running pg_dump?

There was an off-list complaint that if you repeat the ALTER .. DEPENDS
for the same object on the same extension, then the same dependency is
registered multiple times.  (You can search pg_depend for "deptype = 'x'"
to see that).  I suppose that would lead to the line being output
multiple times by pg_dump, also.  Is that what you did?

If so: Patch 0002 is supposed to fix that problem, by raising an error
if the dependency is already registered ... though it occurs to me now
that it would be more in line with custom to make the command a silent
no-op.  In fact, doing that would cause old dumps (generated with
databases containing duplicated entries) to correctly restore a single
entry, without error.  Therefore my inclination now is to change 0002
that way and push and backpatch it ahead of 0001.

I realize just now that I have failed to verify what happens with
partitioned indexes.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            not tested

It works for me

On 2020-Mar-05, Ibrar Ahmed wrote:

> Is this intentional that there is no error when removing a non-existing
> dependency?

Hmm, I think we can do nothing silently if nothing is called for.
So, yes, that seems to be the way it should work.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020-Mar-05, Alvaro Herrera wrote:

> On 2020-Mar-05, Ibrar Ahmed wrote:
> 
> > Is this intentional that there is no error when removing a non-existing
> > dependency?
> 
> Hmm, I think we can do nothing silently if nothing is called for.
> So, yes, that seems to be the way it should work.

I pushed 0002 to all branches (9.6+), after modifying it to silently do
nothing instead of throwing an error when the dependency exists -- same
we discussed here, for the other form of the command.
I just noticed that I failed to credit Ahsan Hadi as reviewer for this
patch :-(

Thanks for reviewing.  I'll see about 0001 next, also backpatched to
9.6.

I'm still not sure whether to apply 0003 (+ your tab-completion patch,
thanks for it) to backbranches or just to master.  It seems legitimate
to see it as a feature addition, but OTOH the overall feature is not
complete without it ...

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> I'm still not sure whether to apply 0003 (+ your tab-completion patch,
> thanks for it) to backbranches or just to master.  It seems legitimate
> to see it as a feature addition, but OTOH the overall feature is not
> complete without it ...

0003 is the command addition to allow removing such a dependency,
right?  Given the lack of field demand I see no reason to risk
adding it to the back branches.

BTW, I did not like the syntax too much.  "NO DEPENDS ON EXTENSION"
doesn't seem like good English.  "NOT DEPENDS ON EXTENSION" is hardly
any better.  The real problem with both is that an ALTER action should
be, well, an action.  A grammar stickler would say that it should be
"ALTER thing DROP DEPENDENCY ON EXTENSION ext", but perhaps we could
get away with "ALTER thing DROP DEPENDS ON EXTENSION ext" to avoid
adding a new keyword.  By that logic the original command should have
been "ALTER thing ADD DEPENDS ON EXTENSION ext", but I suppose it's
too late for that.

            regards, tom lane

Thanks for the reviews; I pushed 0001 now, again to all branches since
9.6.  Because of the previous commit, the fact that multiple statements
are emitted is not important anymore: the server will only restore the
first one, and silently ignore subsequent ones.  And once you're using a
system in that state, naturally only one will be emitted by pg_dump in
all cases.

What remains on this CF item is the new feature to remove an existing
dependency.  As Tom says, given the little use this feature gets it
doesn't sound worth the destabilization risk in back branches, so I'm
going to push it only to master -- but not yet.

Thanks,

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020-Mar-11, Tom Lane wrote:

> > thanks for it) to backbranches or just to master.  It seems legitimate
> > to see it as a feature addition, but OTOH the overall feature is not
> > complete without it ...
> 
> 0003 is the command addition to allow removing such a dependency,
> right?  Given the lack of field demand I see no reason to risk
> adding it to the back branches.

Yeah, okay.

I hereby request permission to push this patch past the feature freeze
date; it's a very small one that completes an existing feature (rather
than a complete new feature in itself), and it's not intrusive nor
likely to break anything.

> BTW, I did not like the syntax too much.  "NO DEPENDS ON EXTENSION"
> doesn't seem like good English.  "NOT DEPENDS ON EXTENSION" is hardly
> any better.  The real problem with both is that an ALTER action should
> be, well, an action.  A grammar stickler would say that it should be
> "ALTER thing DROP DEPENDENCY ON EXTENSION ext", but perhaps we could
> get away with "ALTER thing DROP DEPENDS ON EXTENSION ext" to avoid
> adding a new keyword.  By that logic the original command should have
> been "ALTER thing ADD DEPENDS ON EXTENSION ext", but I suppose it's
> too late for that.

I will be submitting a version with these changes shortly.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020-Mar-11, Tom Lane wrote:

> BTW, I did not like the syntax too much.  "NO DEPENDS ON EXTENSION"
> doesn't seem like good English.  "NOT DEPENDS ON EXTENSION" is hardly
> any better.  The real problem with both is that an ALTER action should
> be, well, an action.  A grammar stickler would say that it should be
> "ALTER thing DROP DEPENDENCY ON EXTENSION ext", but perhaps we could
> get away with "ALTER thing DROP DEPENDS ON EXTENSION ext" to avoid
> adding a new keyword.  By that logic the original command should have
> been "ALTER thing ADD DEPENDS ON EXTENSION ext", but I suppose it's
> too late for that.

The problem with DROP DEPENDS is alter_table_cmd, which already defines
"DROP opt_column ColId", so there's a reduce/reduce conflict for the
ALTER INDEX and ALTER MATERIALIZED VIEW forms because "depends" could be
a column name.  (It works fine for ALTER FUNCTION/ROUTINE/PROCEDURE/TRIGGER
because there's no command that tries to define a conflicting DROP form
for these.)

It works if I change DEPENDS to be type_func_name_keyword (currently
unreserved_keyword), but I bet we won't like that.

(DEPENDENCY is not a keyword of any kind, so DROP DEPENDENCY require us
making it one of high reservedness, which I suspect we don't like
either).

It would also work to use a different keyword in the DROP position;
maybe REMOVE.  But that's not a keyword currently.

How about ALTER ..  REVOKE DEPENDS or DELETE DEPENDS?  Bison is okay
with either of those forms.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

As promised, here's a rebased version of this patch -- edits pending per
discussion to decide the grammar to use.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

I pushed this (to pg13 only) using the originally proposed "NO DEPENDS"
syntax.  It's trivial to change to REVOKE DEPENDS on REMOVE DEPENDS if
we decide to do that.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020-Mar-06, Ibrar Ahmed wrote:

> I think we need a tab-completion patch too for this.

Thanks, I pushed this.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services