Thread: add test: pg_rowlocks extension

Hi Hackers,
I just wrote test about pg_rowlocks extension.
I added sql and spec test for locking state.

---
Regards
DongWook Lee

Dong Wook Lee <sh95119@gmail.com> writes:
> I just wrote test about pg_rowlocks extension.
> I added sql and spec test for locking state.

I think this could be cut down quite a bit.  Do we really need
both a SQL test and an isolation test?  Seems like you could
easily do everything in the isolation test.

Also, it is not a good idea to go creating superusers in a contrib
test: we support "make installcheck" for these tests, but people don't
especially like new superusers cropping up in their installations.
I doubt that we need *any* of the permissions-ish tests that you
propose adding here; those are not part of the module's own
functionality, and we don't generally have similar tests in other
contrib modules.

If you do keep any of it, remember to drop the roles you create ---
leaving global objects behind is not OK.  (For one thing, it
breaks doing repeat "make installcheck"s.)

Another thing that's bad style is the "drop table if exists".
This should be running in an empty database, and if somehow it's
not, destroying pre-existing objects would be pretty unfriendly.
Better to fail at the CREATE.

See also my comments about your pg_buffercache patch, which
largely apply here too.

            regards, tom lane

2022년 7월 31일 (일) 오전 6:32, Tom Lane <tgl@sss.pgh.pa.us>님이 작성:
>
> Dong Wook Lee <sh95119@gmail.com> writes:
> > I just wrote test about pg_rowlocks extension.
> > I added sql and spec test for locking state.
>
> I think this could be cut down quite a bit.  Do we really need
> both a SQL test and an isolation test?  Seems like you could
> easily do everything in the isolation test.

I agree with your optionion.

> Also, it is not a good idea to go creating superusers in a contrib
> test: we support "make installcheck" for these tests, but people don't
> especially like new superusers cropping up in their installations.
> I doubt that we need *any* of the permissions-ish tests that you
> propose adding here; those are not part of the module's own
> functionality, and we don't generally have similar tests in other
> contrib modules.

I agree it's right to remove that part.

> If you do keep any of it, remember to drop the roles you create ---
> leaving global objects behind is not OK.  (For one thing, it
> breaks doing repeat "make installcheck"s.)
>
> Another thing that's bad style is the "drop table if exists".
> This should be running in an empty database, and if somehow it's
> not, destroying pre-existing objects would be pretty unfriendly.
> Better to fail at the CREATE.

Thank you for the good explanation. It will be very helpful to write a
test in the future.

> See also my comments about your pg_buffercache patch, which
> largely apply here too.
OK. I will add the `.gitignore` file.

I will revise my patch and submit it again as soon as possible.

I modified my previous patch by reflecting the feedback.
and I wrote most of the queries for the test after looking at the file below.

- ref: (https://github.com/postgres/postgres/blob/master/src/test/isolation/specs/tuplelock-conflict.spec)

The coverage of the test is approximately 81.5%.

If there is any problem, I would appreciate it if you let me know anytime.
Thank you always for your kind reply.

Dong Wook Lee <sh95119@gmail.com> writes:
> I modified my previous patch by reflecting the feedback.
> and I wrote most of the queries for the test after looking at the file below.

Pushed with some revisions.  Notably, I didn't see any point in
repeating each test case four times, so I trimmed it down to once
per case.

            regards, tom lane

On Fri, Sep 2, 2022 at 4:07 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Pushed with some revisions.  Notably, I didn't see any point in
> repeating each test case four times, so I trimmed it down to once
> per case.

I checked it.
Thank you for correcting it in a better way.