Dear Amit, > OTOH, if we don't want to adjust physical > slot machinery, it seems saving the logical slots to disk immediately > when its restart_lsn is updated is a waste of effort after your patch, > no? If so, why are we okay with that?
I agree, that saving logical slots at advance is a possible waste of effort. But I don't understand original ideas behind it. I haven't touched it to make the minimal patch which should not break the existing functionality. We trim WAL in checkpoint (or restart point) operation only. The slots' restart_lsn is used to keep the wal from truncation. I believe, we need to compute the slots' oldest lsn as the minimal value of restart_lsn values only when executing checkpoint (or restart point). I guess, it doesn't depend on slot's type (logical or physical). We have 0003 patch to fix it. I haven't deeply investigated yet slot's xmin values but I guess the xmin values are a different story than restart_lsn. It is used to avoid tuple deletions by vacuum and it is updated by a different way. I can't say that LogicalConfirmReceivedLocation is the right place to update saved on disk xmin values. I would propose to update these values in SaveSlotToPath under some lock to avoid concurrent reads of unsaved values or do in a checkpoint like as for restart_lsn. We may investigate and improve it in an another patch. With best regards, Vitaly
