`PgRelation::heap_relation` should have a `heap_relation_with_lock` alternative

[ccleve]

PgRelation has this code in it:

    pub fn heap_relation(&self) -> Option<PgRelation> {
        // SAFETY: we know self.boxed and its members are correct as we created it
        let rd_index: PgBox<pg_sys::FormData_pg_index> =
            unsafe { PgBox::from_pg(self.boxed.rd_index) };
        if rd_index.is_null() {
            None
        } else {
            Some(unsafe { PgRelation::open(rd_index.indrelid) })
        }
    }

PgRelation::open() does not lock the heaprel, which means that if you use it for anything and another process tries to modify it, you might get a race condition. Am I understanding this correctly?

Would it not make more sense to replace the PgRelation::open() line with:

Some(unsafe{PgRelation::with_lock(rd_index.indrelid, self.lockmode)})

I hesitate to submit a PR in case I've missed something here.

[eeeebbbbrrrr]

We should probably change this function to be unsafe and document why. Postgres internally documents its RelationIdGetRelation (which I believe is what PgRelation::open() uses) as something like "the caller must already have an appropriate lock or bad things can happen", so something along the lines of what Postgres says.

Then we add a new fn heap_relation_with_lock(&self, lockmode: pg_sys::LOCKMODE) that calls PgRelation::with_lock() as you suggest.

The first would be a breaking change but yeah, the code in pgrx is clearly wrong. :(

[workingjubilee]

oh dear.

[workingjubilee]

I have posted #2176 to address the first part of "make it unsafe". It will be one of the many reasons 0.17 is a major release. I am not sure whether we should add a LockGuard type of some sort on the Rust side here for the locks or what, so I am not doing the second part as I feel it merits more design thought than a quick glance.