Thread: [HACKERS] Why are we restricting exported snapshots in subtransactions?
Hi, ExportSnapshot() has, right at the beginning, the following block: /* * We cannot export a snapshot from a subtransaction because there's no * easy way for importers to verify thatthe same subtransaction is still * running. */ if (IsSubTransaction()) ereport(ERROR, (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION), errmsg("cannot export a snapshot from a subtransaction"))); that reasoning doesn't seem to make too much sense to me. Given that exported snapshots don't make the exporting-transaction's changes visible, I don't see why that restriction is needed? As long as the exported snapshot enforces xmin to be retained, which it does via the pairingheap, I don't understand why we'd have to enforce that the subtransaction is still running? I don't have any need for that capability right now, thus am not planning to submit a patch changing this, but I'm about to apply a patch to ExportSnapshot() to address one of the v10 open items, so I'd like to make sure I understand the constraints. Greetings, Andres Freund
On Mon, Jun 12, 2017 at 11:04 PM, Andres Freund <andres@anarazel.de> wrote: > ExportSnapshot() has, right at the beginning, the following block: > > /* > * We cannot export a snapshot from a subtransaction because there's no > * easy way for importers to verify that the same subtransaction is still > * running. > */ > if (IsSubTransaction()) > ereport(ERROR, > (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION), > errmsg("cannot export a snapshot from a subtransaction"))); > > that reasoning doesn't seem to make too much sense to me. Given that > exported snapshots don't make the exporting-transaction's changes > visible, I don't see why that restriction is needed? > > As long as the exported snapshot enforces xmin to be retained, which it > does via the pairingheap, I don't understand why we'd have to enforce > that the subtransaction is still running? I think you're touching on what is perhaps the key issue here, which is that if it were possible to remove a tuple that the snapshot ought to see before the snapshot got used, that would be bad. I don't immediately see why we couldn't remove a tuple inserted by the aborted subtransaction immediately. On a quick look, it seems to me that HeapTupleSatisfiesVacuum() could fall through all the way to this case: /* * Not in Progress, Not Committed, so either Aborted or crashed */ SetHintBits(tuple, buffer, HEAP_XMIN_INVALID, InvalidTransactionId); return HEAPTUPLE_DEAD; Even If I'm wrong about that, it seems like someone might want to make me correct in the future - i.e. removing aborted tuples ASAP seems like a good idea. Another point to consider is whether a relfilenode assignment visible to that snapshot might be a file that's since been truncated or removed altogether. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2017-06-13 13:15:57 -0400, Robert Haas wrote: > On Mon, Jun 12, 2017 at 11:04 PM, Andres Freund <andres@anarazel.de> wrote: > > ExportSnapshot() has, right at the beginning, the following block: > > > > /* > > * We cannot export a snapshot from a subtransaction because there's no > > * easy way for importers to verify that the same subtransaction is still > > * running. > > */ > > if (IsSubTransaction()) > > ereport(ERROR, > > (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION), > > errmsg("cannot export a snapshot from a subtransaction"))); > > > > that reasoning doesn't seem to make too much sense to me. Given that > > exported snapshots don't make the exporting-transaction's changes > > visible, I don't see why that restriction is needed? > > > > As long as the exported snapshot enforces xmin to be retained, which it > > does via the pairingheap, I don't understand why we'd have to enforce > > that the subtransaction is still running? > > I think you're touching on what is perhaps the key issue here, which > is that if it were possible to remove a tuple that the snapshot ought > to see before the snapshot got used, that would be bad. I don't think that's really an issue here. Exported snapshots export a state of the database of that moment, *except* that changes made by the exporting transaction are not visible. IOW, a subtransaction's changes aren't going to be visible anyway. As far as I can tell that property makes: > I don't > immediately see why we couldn't remove a tuple inserted by the aborted > subtransaction immediately. On a quick look, it seems to me that > HeapTupleSatisfiesVacuum() could fall through all the way to this > case: > Even If I'm wrong about that, it seems like someone might want to make > me correct in the future - i.e. removing aborted tuples ASAP seems > like a good idea. > Another point to consider is whether a relfilenode assignment visible > to that snapshot might be a file that's since been truncated or > removed altogether. All pretty much moot? The reason subxids are exported right now is to *ignore* changes made by them. But for that we could just as well set suboverflowed to true, and just include the toplevel xid in ->xip[]. - Andres