Re: deparsing utility commands
| От | Shulgin, Oleksandr |
|---|---|
| Тема | Re: deparsing utility commands |
| Дата | |
| Msg-id | CACACo5Q_UXYwF117LBhjZ3xaMPyrgqnqE=mXvRhEfjJ51aCfwQ@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: deparsing utility commands (Alvaro Herrera <alvherre@2ndquadrant.com>) |
| Ответы |
Re: deparsing utility commands
(Ajin Cherian <itsajin@gmail.com>)
Re: deparsing utility commands (Ajin Cherian <itsajin@gmail.com>) |
| Список | pgsql-hackers |
On Thu, Aug 20, 2015 at 6:46 PM, Alvaro Herrera <alvherre@2ndquadrant.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
Вложения
В списке pgsql-hackers по дате отправления: