pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedFallout from 'Make type "name" collation-aware' and tg_table_name
3+ messages / 2 participants
[nested] [flat]
* Fallout from 'Make type "name" collation-aware' and tg_table_name
@ 2022-06-08 13:07 Christoph Berg <christoph.berg@credativ.de>
0 siblings, 1 reply; 3+ messages in thread
From: Christoph Berg @ 2022-06-08 13:07 UTC (permalink / raw)
To: pgsql-bugs
Hi,
this is more a heads-up than a bug report, but I'm still reporting it
here because I think that variant of the problem has not been
addressed in the releases notes.
I was just debugging a client problem where a trigger was running fine
with PG 11, but very slow with PG 13. The symptom is that some COLLATE "C"
is appearing in the auto_explain output:
Good trigger plan on PG 11:
-> Index Scan using cache_revisions_zone_table_rev on cache_revisions (cost=0.15..8.17 rows=1 width=218) (actual time=0.009..0.009 rows=0 loops=1)
Index Cond: (((zone_id)::text = 'insert-test'::text) AND ((table_name)::text = 'provider'::text) AND (revision < 1))
Buffers: shared hit=1
Bad trigger plan on PG 13 (and PG 12):
-> Seq Scan on cache_revisions (cost=0.00..15.60 rows=1 width=218) (actual time=0.007..0.007 rows=0 loops=1)
Filter: ((revision < 1) AND ((table_name)::text = 'provider'::text COLLATE "C") AND ((zone_id)::text = 'insert-test'::text COLLATE "C"))
Rows Removed by Filter: 1
Buffers: shared hit=1
Git-bisecting leads to this commit introducing the change:
commit 586b98fdf1aaef4a27744f8b988479aad4bd9a01
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: Wed Dec 19 17:35:12 2018 -0500
Make type "name" collation-aware.
The "name" comparison operators now all support collations, making them
functionally equivalent to "text" comparisons, except for the different
physical representation of the datatype. They do, in fact, mostly share
the varstr_cmp and varstr_sortsupport infrastructure, which has been
slightly enlarged to handle the case.
To avoid changes in the default behavior of the datatype, set name's
typcollation to C_COLLATION_OID not DEFAULT_COLLATION_OID, so that
by default comparisons to a name value will continue to use strcmp
semantics. (This would have been the case for system catalog columns
anyway, because of commit 6b0faf723, but doing this makes it true for
user-created name columns as well. In particular, this avoids
locale-dependent changes in our regression test results.)
In consequence, tweak a couple of places that made assumptions about
collatable base types always having typcollation DEFAULT_COLLATION_OID.
I have not, however, attempted to relax the restriction that user-
defined collatable types must have that. Hence, "name" doesn't
behave quite like a user-defined type; it acts more like a domain
with COLLATE "C". (Conceivably, if we ever get rid of the need for
catalog name columns to be fixed-length, "name" could actually become
such a domain over text. But that'd be a pretty massive undertaking,
and I'm not volunteering.)
Discussion: https://postgr.es/m/15938.1544377821@sss.pgh.pa.us
And indeed the "name" type is used in the trigger code, via
tg_table_name:
CREATE FUNCTION public.update_cache_revision() RETURNS trigger
LANGUAGE plpgsql SECURITY DEFINER
AS $$
DECLARE
v_zone_id TEXT;
BEGIN
v_zone_id := NEW.zone_id;
PERFORM increase_revision(tg_table_name::TEXT, v_zone_id);
RETURN NEW;
END;
$$;
The "C" collation of name on tg_table_name is preserved by the ::text
cast, and the code in increase_revision() then fails to use the btree
index on cache_revisions (where the column is of type "text") because
the collation does not match.
The intent of the change seems to have been not to change the
user-visible behavior, but here it's clearly changed when leaving the
collation at "default" would have meant no change.
Of course the fix is to attach 'collate "default"' to tg_table_name:
PERFORM increase_revision(tg_table_name::TEXT collate "default", v_zone_id);
The PG 12 release notes have this note:
<para>
Mark table columns of type <link
linkend="datatype-character-special-table">name</link> as having
<quote>C</quote> collation by default (Tom Lane, Daniel Vérité)
</para>
<para>
The comparison operators for data type <type>name</type> can now use
any collation, rather than always using <quote>C</quote> collation.
To preserve the previous semantics of queries, columns of
type <type>name</type> are now explicitly marked as
having <quote>C</quote> collation. A side effect of this is that
regular-expression operators on <type>name</type> columns will now
use the <quote>C</quote> collation by default, not the database
collation, to determine the behavior of locale-dependent regular
expression patterns (such as <literal>\w</literal>). If you want
non-C behavior for a regular expression on a <type>name</type>
column, attach an explicit <literal>COLLATE</literal> clause. (For
user-defined <type>name</type> columns, another possibility is to
specify a different collation at table creation time; but that just
moves the non-backwards-compatibility to the comparison operators.)
</para>
Of course it's a bit too late now to change that, but the impact is
clearly wider than just regular expression operations.
Christoph
--
Senior Consultant, Tel.: +49 2166 9901 187
credativ GmbH, HRB Mönchengladbach 12080, USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Geoff Richardson, Peter Lilley
Unser Umgang mit personenbezogenen Daten unterliegt folgenden
Bestimmungen: https://www.credativ.de/datenschutz
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Fallout from 'Make type "name" collation-aware' and tg_table_name
@ 2022-06-08 14:05 Tom Lane <tgl@sss.pgh.pa.us>
parent: Christoph Berg <christoph.berg@credativ.de>
0 siblings, 1 reply; 3+ messages in thread
From: Tom Lane @ 2022-06-08 14:05 UTC (permalink / raw)
To: Christoph Berg <christoph.berg@credativ.de>; +Cc: pgsql-bugs
Christoph Berg <christoph.berg@credativ.de> writes:
> I was just debugging a client problem where a trigger was running fine
> with PG 11, but very slow with PG 13.
Hmmm ... I do not see how the collation within the PERFORM statement
would affect whatever it's calling?
regards, tom lane
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Fallout from 'Make type "name" collation-aware' and tg_table_name
@ 2022-06-08 14:12 Christoph Berg <christoph.berg@credativ.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 3+ messages in thread
From: Christoph Berg @ 2022-06-08 14:12 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs
Re: Tom Lane
> Christoph Berg <christoph.berg@credativ.de> writes:
> > I was just debugging a client problem where a trigger was running fine
> > with PG 11, but very slow with PG 13.
>
> Hmmm ... I do not see how the collation within the PERFORM statement
> would affect whatever it's calling?
Here's the actual reproducer I used:
$ cat run.sql
drop database if exists test;
create database test;
\c test
\i pgdump
reset all;
alter system set log_line_prefix = '%m [%p] %q%u@%d %a ';
select pg_reload_conf();
set jit=off;
LOAD 'auto_explain';
set auto_explain.log_analyze=on;
set auto_explain.log_buffers=on;
set auto_explain.log_min_duration=0;
set auto_explain.log_nested_statements=on;
set auto_explain.log_triggers=on;
set application_name = 'collate_bug';
INSERT INTO public.identity_provider (id, created, lastmodified, "version", identity_zone_id, "name", origin_key, "type", config, active)
VALUES('24e9dd6d-5a63-9ea8-b425-ae89aa419a33', CURRENT_TIMESTAMP,CURRENT_TIMESTAMP, 0, 'insert-test', 'test', 'test', 'test', 'none', true);
Mit freundlichen Grüßen,
Christoph Berg
--
Senior Consultant, Tel.: +49 2166 9901 187
credativ GmbH, HRB Mönchengladbach 12080, USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Geoff Richardson, Peter Lilley
Unser Umgang mit personenbezogenen Daten unterliegt folgenden
Bestimmungen: https://www.credativ.de/datenschutz
Attachments:
pgdump (text/plain, 5.8K) download
--
-- PostgreSQL database dump
--
-- Dumped from database version 13.6 (Debian 13.6-1.pgdg110+1)
-- Dumped by pg_dump version 13.7 (Ubuntu 13.7-1.pgdg20.04+1)
SET statement_timeout = 0;
SET lock_timeout = 0;
SET idle_in_transaction_session_timeout = 0;
SET client_encoding = 'UTF8';
SET standard_conforming_strings = on;
SELECT pg_catalog.set_config('search_path', '', false);
SET check_function_bodies = false;
SET xmloption = content;
SET client_min_messages = warning;
SET row_security = off;
--
-- Name: increase_revision(text, text); Type: FUNCTION; Schema: public; Owner: root
--
CREATE FUNCTION public.increase_revision(v_table_name text, v_identity_zone_id text) RETURNS integer
LANGUAGE plpgsql
AS $$
DECLARE
rev INTEGER;
updated_rows INTEGER;
BEGIN
SELECT nextval('xs_cache_revisions_seq') INTO rev;
UPDATE xs_cache_revisions
SET revision = rev
WHERE table_name = v_table_name
AND identity_zone_id = v_identity_zone_id
AND revision = (SELECT revision
FROM xs_cache_revisions
WHERE table_name = v_table_name
AND identity_zone_id = v_identity_zone_id FOR UPDATE SKIP LOCKED LIMIT 1);
GET DIAGNOSTICS updated_rows = ROW_COUNT;
IF (updated_rows = 0) THEN
INSERT INTO xs_cache_revisions(table_name, identity_zone_id, revision)
values (v_table_name, v_identity_zone_id, rev);
END IF;
delete from xs_cache_revisions where (table_name,identity_zone_id,revision) in (select table_name,identity_zone_id,revision from xs_cache_revisions where (table_name = v_table_name and identity_zone_id = v_identity_zone_id and revision < rev) for update SKIP LOCKED);
RETURN rev;
END;
$$;
--ALTER FUNCTION public.increase_revision(v_table_name text, v_identity_zone_id text) OWNER TO root;
--
-- Name: update_cache_revision(); Type: FUNCTION; Schema: public; Owner: root
--
CREATE FUNCTION public.update_cache_revision() RETURNS trigger
LANGUAGE plpgsql SECURITY DEFINER
AS $$
DECLARE
v_zone_id TEXT;
BEGIN
IF (TG_OP = 'UPDATE') or (TG_OP = 'INSERT') THEN
v_zone_id := NEW.identity_zone_id;
PERFORM increase_revision(tg_table_name::TEXT, v_zone_id);
RETURN NEW;
ELSIF (TG_OP = 'DELETE') THEN
v_zone_id := OLD.identity_zone_id;
PERFORM increase_revision(tg_table_name::TEXT, v_zone_id);
RETURN OLD;
END IF;
END;
$$;
--ALTER FUNCTION public.update_cache_revision() OWNER TO root;
SET default_tablespace = '';
--SET default_table_access_method = heap;
--
-- Name: identity_provider; Type: TABLE; Schema: public; Owner: root
--
CREATE TABLE public.identity_provider (
id character varying(36) NOT NULL,
created timestamp without time zone DEFAULT CURRENT_TIMESTAMP,
lastmodified timestamp without time zone DEFAULT CURRENT_TIMESTAMP,
version bigint DEFAULT 0,
identity_zone_id character varying(36) NOT NULL,
name character varying(255) NOT NULL,
origin_key character varying(36) NOT NULL,
type character varying(255) NOT NULL,
config text,
active boolean DEFAULT true NOT NULL
);
--ALTER TABLE public.identity_provider OWNER TO root;
--
-- Name: xs_cache_revisions; Type: TABLE; Schema: public; Owner: root
--
CREATE TABLE public.xs_cache_revisions (
table_name character varying(48) NOT NULL,
identity_zone_id character varying(36) NOT NULL,
revision bigint NOT NULL,
created timestamp without time zone DEFAULT now()
);
--ALTER TABLE public.xs_cache_revisions OWNER TO root;
--
-- Name: xs_cache_revisions_revision_seq; Type: SEQUENCE; Schema: public; Owner: root
--
CREATE SEQUENCE public.xs_cache_revisions_revision_seq
START WITH 1
INCREMENT BY 1
NO MINVALUE
NO MAXVALUE
CACHE 1;
--ALTER TABLE public.xs_cache_revisions_revision_seq OWNER TO root;
--
-- Name: xs_cache_revisions_revision_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: root
--
ALTER SEQUENCE public.xs_cache_revisions_revision_seq OWNED BY public.xs_cache_revisions.revision;
--
-- Name: xs_cache_revisions_seq; Type: SEQUENCE; Schema: public; Owner: root
--
CREATE SEQUENCE public.xs_cache_revisions_seq
START WITH 1
INCREMENT BY 1
NO MINVALUE
NO MAXVALUE
CACHE 1;
--ALTER TABLE public.xs_cache_revisions_seq OWNER TO root;
--
-- Name: xs_cache_revisions revision; Type: DEFAULT; Schema: public; Owner: root
--
ALTER TABLE ONLY public.xs_cache_revisions ALTER COLUMN revision SET DEFAULT nextval('public.xs_cache_revisions_revision_seq'::regclass);
--
-- Name: identity_provider identity_provider_pkey; Type: CONSTRAINT; Schema: public; Owner: root
--
ALTER TABLE ONLY public.identity_provider
ADD CONSTRAINT identity_provider_pkey PRIMARY KEY (id);
--
-- Name: xs_cache_revisions xs_cache_revisions_constr; Type: CONSTRAINT; Schema: public; Owner: root
--
ALTER TABLE ONLY public.xs_cache_revisions
ADD CONSTRAINT xs_cache_revisions_constr UNIQUE (table_name, identity_zone_id, revision);
--
-- Name: active_in_zone; Type: INDEX; Schema: public; Owner: root
--
CREATE INDEX active_in_zone ON public.identity_provider USING btree (identity_zone_id, active);
--
-- Name: key_in_zone; Type: INDEX; Schema: public; Owner: root
--
CREATE UNIQUE INDEX key_in_zone ON public.identity_provider USING btree (identity_zone_id, origin_key);
--
-- Name: xs_cache_revisions_zone_table_rev; Type: INDEX; Schema: public; Owner: root
--
CREATE INDEX xs_cache_revisions_zone_table_rev ON public.xs_cache_revisions USING btree (identity_zone_id, table_name, revision);
--
-- Name: identity_provider t_identity_provider_change_trigger; Type: TRIGGER; Schema: public; Owner: root
--
CREATE TRIGGER t_identity_provider_change_trigger AFTER INSERT OR DELETE OR UPDATE ON public.identity_provider FOR EACH ROW EXECUTE PROCEDURE public.update_cache_revision('false');
--
-- PostgreSQL database dump complete
--
end of thread, other threads:[~2022-06-08 14:12 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-06-08 13:07 Fallout from 'Make type "name" collation-aware' and tg_table_name Christoph Berg <christoph.berg@credativ.de>
2022-06-08 14:05 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-06-08 14:12 ` Christoph Berg <christoph.berg@credativ.de>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox