Re: deparsing utility commands

On Thu, Aug 20, 2015 at 6:46 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> Shulgin, Oleksandr wrote:
>
> > A particularly nasty one is:
> >
> > ERROR: index "cwi_replaced_pkey" does not exist
> >
> > The test statement that's causing it is this one:
> >
> > ALTER TABLE cwi_test DROP CONSTRAINT cwi_uniq_idx,
> > ADD CONSTRAINT cwi_replaced_pkey PRIMARY KEY
> > USING INDEX cwi_uniq2_idx;
> >
> > Which gets deparsed as:
> >
> > ALTER TABLE cwi_test DROP CONSTRAINT cwi_uniq_idx,
> > ADD CONSTRAINT cwi_replaced_pkey PRIMARY KEY
> > USING INDEX cwi_replaced_pkey;
> >
> > The problem is that during constraint transformation, the index is being
> > renamed to match the constraint name and at the deparse stage the
> original
> > index name appears to be lost completely... I haven't figure out if
> > there's a way around unless we introduce a new field in IndexStmt
> > (originalName?) to cover exactly this corner case.
>
> Oh :-( Hmm, modifying parse nodes should normally be considered last
> resort, and at the same time identifying objects based on names rather
> than OIDs is frowned upon. But I don't see any way to handle this
> otherwise. I'm not sure what's the best solution for this one.

Yeah, me neither. At least one can see that in the Constraint struct we
have both indexname and conname, so adding conname to IndexStmt doesn't
seem to be really in disconnect with that (especially since we're
constructing an IndexStmt from a Constraint in this case). See patch #5.

Or maybe we should drop the isconstraint/deferrable/initdeferred fields and
put there an optional pointer to Constraint struct instead? Just an idea,
I didn't check how intrusive such a change might be.

> The updated versions of the core-support patch and the contrib modules are
> > attached.
>
> Please try to split things up, or at least don't mix more than it
> already is. For instance, the tsconfig mapping stuff should be its own
> patch; we can probably make a case for pushing that on its own.
>
> Also I see you added one caller of EventTriggerInhibitCommandCollection.
> I don't like that crap at all and I think we would be better off if we
> were able to remove it completely. Please see whether it's possible to
> handle this case in some other way.
>

Well, I've only added it to suppress the extra SET NOT NULL sub-commands
produced by the original ADD CONSTRAINT PRIMARY KEY command. The deparsed
command is still correct I believe, though a bit too verbose:

ALTER TABLE public.cwi_test
ALTER COLUMN a SET NOT NULL,
ALTER COLUMN b SET NOT NULL,
ADD CONSTRAINT cwi_uniq_idx PRIMARY KEY USING INDEX cwi_uniq_idx NOT
DEFERRABLE INITIALLY IMMEDIATE;

The only other place where this inhibiting is employed is around refresh
matview command and it prevents extra plumbing output like this:

CREATE TEMPORARY TABLE pg_temp.pg_temp_32070_2 WITH (oids=OFF) AS
SELECT mv.ctid AS tid, newdata.*::pg_temp_32070 AS newdata FROM (...);

Now, what I think would make the most sense is still capture all the
intermediary low-level commands, but put them hierarchically under the
command invoking them, so that interested clients can still inspect them,
but for the purpose of reconstructing of captured DDL events one needs to
replay only the top-level commands.

From what I can see, this can be achieved by removing check on
isCompleteQuery in ProcessUtilitySlow and run all EventTrigger*()
regardless, which will stack the captured commands as they are being
captured.

The question when arises is how to reflect the command hierarchy in the
output of pg_event_trigger_ddl_commands(). Maybe simply assigning an
integer id and referencing it in an additional parent_command_id column
would do the trick.

> Another quirk of ALTER TABLE is that due to multi-pass processing in
> > ATRewriteCatalogs, the same command might be collected a number of times.
> > For example, in src/test/regress/sql/inherit.sql:
> >
> > alter table a alter column aa type integer using bit_length(aa);
> >
> > the "alter column type" then appears 4 times in the deparsed output as
> > identical subcommands of a single ALTER TABLE command.

> Yeah, I had a hack somewhere in the collection code that if the relation
> ID was different from what was specified, then the command was ignored.
> I removed that before commit because it seemed possible that for some
> cases you actually want the command reported separately for each child.
>

> I think our best option in this case is to ignore commands that are
> reported for different relations during JSON deparsing. Not sure how
> easy that is.

Hm, but there is no different relation in this case, it's just that we get
the "ALTER COLUMN ... SET DATA TYPE" sub-command gets repeated multiple
times:

ALTER TABLE public.a
ALTER COLUMN aa SET DATA TYPE pg_catalog.int4 USING
pg_catalog.bit_length(aa),
ALTER COLUMN aa SET DATA TYPE pg_catalog.int4 USING
pg_catalog.bit_length(aa),
ALTER COLUMN aa SET DATA TYPE pg_catalog.int4 USING
pg_catalog.bit_length(aa),
ALTER COLUMN aa SET DATA TYPE pg_catalog.int4 USING
pg_catalog.bit_length(aa),
ALTER COLUMN aa SET DATA TYPE pg_catalog.int4 USING
pg_catalog.bit_length(aa);

Oh, five times actually...

You seem to have squashed the patches? Please keep the split out.
>

Well, if that makes the review process easier :-)

--
Alex