Re: logical changeset generation v4 - Heikki's thoughts about the patch state

On 13-01-24 11:15 AM, Steve Singer wrote:
> On 13-01-24 06:40 AM, Andres Freund wrote:
>>
>> Fair enough. I am also working on a user of this infrastructure but that
>> doesn't help you very much. Steve Singer seemed to make some stabs at
>> writing an output plugin as well. Steve, how far did you get there?
>
> I was able to get something that generated output for INSERT 
> statements in a format similar to what a modified slony apply trigger 
> would want.  This was with the list of tables to replicate hard-coded 
> in the plugin.  This was with the patchset from the last commitfest.I 
> had gotten a bit hung up on the UPDATE and DELETE support because 
> slony allows you to use an arbitrary user specified unique index as 
> your key.  It looks like better support for tables with a unique 
> non-primary key is in the most recent patch set.  I am hoping to have 
> time this weekend to update my plugin to use parameters passed in on 
> the init and other updates in the most recent version.  If I make some 
> progress I will post a link to my progress at the end of the weekend.  
> My big issue is that I have limited time to spend on this.
>

This isn't a complete review just a few questions I've hit so far that I 
thought I'd ask to see if I'm not seeing something related to updates.


*** a/src/include/catalog/index.h
--- b/src/include/catalog/index.h
*************** extern bool ReindexIsProcessingHeap(Oid
*** 114,117 ****
--- 114,121 ----  extern bool ReindexIsProcessingIndex(Oid indexOid);  extern Oid    IndexGetRelation(Oid indexId, bool
missing_ok);

+ extern void relationFindPrimaryKey(Relation pkrel, Oid *indexOid,
+                                    int16 *nratts, int16 *attnums, Oid 
*atttypids,
+                                    Oid *opclasses);
+  #endif   /* INDEX_H */


I don't see this defined anywhere could it be left over from a previous 
version of the patch?


In decode.c
DecodeUpdate:
+
+   /*
+    * FIXME: need to get/save the old tuple as well if we want primary key
+    * changes to work.
+    */
+   change->newtuple = ReorderBufferGetTupleBuf(reorder);

I also don't see any code in heap_update to find + save the old primary 
key values like you added to heap_delete.   You didn't list "Add ability 
to change the primary key on an UPDATE" in the TODO so I'm wondering if 
I'm missing something.  Is there another way I can bet the primary key 
values for the old_tuple?

Also,

I think the name of the test contrib module was changed but you didn't 
update the make file. This fixes it

diff --git a/contrib/Makefile b/contrib/Makefile
index 1cc30fe..36e6bfe 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -50,7 +50,7 @@ SUBDIRS = \        tcn     \        test_parser \        test_decoding   \
-       test_logical_replication \
+       test_logical_decoding \        tsearch2    \        unaccent    \        vacuumlo    \