Re: Support logical replication of DDLs

On 2/16/23 2:43 PM, Jonathan S. Katz wrote:
> On 2/16/23 2:38 PM, Alvaro Herrera wrote:
>> On 2023-Feb-16, Jonathan S. Katz wrote:
>>
>>> On 2/16/23 12:53 PM, Alvaro Herrera wrote:
>>
>>>> I don't think this is the fault of logical replication.  Consider that
>>>> for the backend server, the function source code is just an opaque
>>>> string that is given to the plpgsql engine to interpret.  So there's no
>>>> way for the logical DDL replication engine to turn this into runnable
>>>> code if the table name is not qualified.
>>>
>>> Sure, that's fair. That said, the example above would fall under a 
>>> "typical
>>> use case", i.e. I'm replicating functions that call tables without 
>>> schema
>>> qualification. This is pretty common, and as logical replication becomes
>>> used for more types of workloads (e.g. high availability), we'll 
>>> definitely
>>> see this.
>>
>> Hmm, I think you're saying that replay should turn check_function_bodies
>> off, and I think I agree with that.
> 
> Yes, exactly. +1

I drilled into this a bit more using the SQL standard bodies (BEGIN 
ATOMIC) to see if there were any other behaviors we needed to account 
for. Overall, it worked well but I ran into one issue.

First, functions with "BEGIN ATOMIC" ignores "check_function_bodies" 
which is by design based on how this feature works. We should still turn 
"check_function_bodies" to "off" though, per above discussion.

In the context of DDL replication, "BEGIN ATOMIC" does support 
schema-unqualified functions, presumably because it includes the parsed 
content?

I created an updated example[1] where I converted the SQL functions to 
use the standard syntax and I returned the table names to be schema 
unqualified. This seemed to work, but I ran into a weird case with this 
function:

CREATE OR REPLACE FUNCTION public.calendar_manage(room_id int, 
calendar_date date)
RETURNS void
LANGUAGE SQL
BEGIN ATOMIC
     WITH delete_calendar AS (
         DELETE FROM calendar
         WHERE
             room_id = $1 AND
             calendar_date = $2
     )
     INSERT INTO calendar (room_id, status, calendar_date, calendar_range)
     SELECT $1, c.status, $2, c.calendar_range
     FROM calendar_generate_calendar($1, tstzrange($2, $2 + 1)) c;
END;

This produced an error on the subscriber, with the following message:

2023-02-16 20:58:24.096 UTC [26864] ERROR:  missing FROM-clause entry 
for table "calendar_1" at character 322
2023-02-16 20:58:24.096 UTC [26864] CONTEXT:  processing remote data for 
replication origin "pg_18658" during message type "DDL" in transaction 
980, finished at 0/C099A7D8
2023-02-16 20:58:24.096 UTC [26864] STATEMENT:  CREATE OR REPLACE 
FUNCTION public.calendar_manage ( IN room_id pg_catalog.int4, IN 
calendar_date pg_catalog.date ) RETURNS pg_catalog.void LANGUAGE sql 
VOLATILE PARALLEL UNSAFE CALLED ON NULL INPUT SECURITY INVOKER COST 100 
BEGIN ATOMIC
	 WITH delete_calendar AS (
	          DELETE FROM public.calendar
	           WHERE ((calendar_1.room_id OPERATOR(pg_catalog.=) 
calendar_manage.room_id) AND (calendar_1.calendar_date 
OPERATOR(pg_catalog.=) calendar_manage.calendar_date))
	         )
	  INSERT INTO public.calendar (room_id, status, calendar_date, 
calendar_range)  SELECT calendar_manage.room_id,
	             c.status,
	             calendar_manage.calendar_date,
	             c.calendar_range
	            FROM 
public.calendar_generate_calendar(calendar_manage.room_id, 
pg_catalog.tstzrange((calendar_manage.calendar_date)::timestamp with 
time zone, ((calendar_manage.calendar_date OPERATOR(pg_catalog.+) 
1))::timestamp with time zone)) c(status, calendar_range);
	END

This seemed to add an additional, incorrect reference to the origin 
table for the "room_id" and "calendar_date" attributes within the CTE of 
this function. I don't know if this is directly related to the DDL 
replication patch, but reporting it as I triggered the behavior through it.

Thanks,

Jonathan

[1] https://gist.github.com/jkatz/fe29006b724fd6f32ee849a96dc01608